New A72 changes for OpenTelemetry #8216 #8226
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8226 +/- ##
==========================================
- Coverage 82.09% 82.06% -0.04%
==========================================
Files 412 412
Lines 40491 40497 +6
==========================================
- Hits 33242 33233 -9
- Misses 5876 5891 +15
Partials 1373 1373
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
was there no change needed for Attempt? Is it already following grpc/proposal#474 ?
stats/opentelemetry/trace.go
Outdated
| @@ -14,6 +14,10 @@ | |||
| * limitations under the License. | |||
| */ | |||
|
|
|||
| // OpenCensus's binary format for grpc-trace-bin: | |||
There was a problem hiding this comment.
no need to add this. This was only for grfc
purnesh42H
added
Type: Internal Cleanup
The Attempt span name already follows the grpc/proposal#474 naming convention. |
|
@dfawley for second review |
stats/opentelemetry/trace.go
Outdated
| attribute.Int64("sequence-number", int64(ai.countRecvMsg)), | ||
| attribute.Int64("message-size", int64(rs.Length)), | ||
| attribute.Int64("message-size-compressed", int64(rs.CompressedLength)), | ||
| )) | ||
| case *stats.OutPayload: | ||
| ai.countSentMsg++ | ||
| span.AddEvent("Outbound compressed message", trace.WithAttributes( | ||
| span.AddEvent("Outbound message", trace.WithAttributes( | ||
| attribute.Int64("sequence-number", int64(ai.countSentMsg)), | ||
| attribute.Int64("message-size", int64(rs.Length)), | ||
| attribute.Int64("message-size-compressed", int64(rs.CompressedLength)), |
There was a problem hiding this comment.
the grfc says to only add this if the message is actually compressed
stats/opentelemetry/trace.go
Outdated
| @@ -58,14 +58,14 @@ func populateSpan(rs stats.RPCStats, ai *attemptInfo) { | |||
| // message id - "must be calculated as two different counters starting | |||
| // from one for sent messages and one for received messages." | |||
| ai.countRecvMsg++ | |||
| span.AddEvent("Inbound compressed message", trace.WithAttributes( | |||
| span.AddEvent("Inbound message", trace.WithAttributes( | |||
| attribute.Int64("sequence-number", int64(ai.countRecvMsg)), | |||
| attribute.Int64("message-size", int64(rs.Length)), | |||
| attribute.Int64("message-size-compressed", int64(rs.CompressedLength)), | |||
There was a problem hiding this comment.
the grfc says to only add this if the message is actually compressed
There was a problem hiding this comment.
gRFC says if compression is not a separate event, just add the key message-size-compressed in the same event "Inbound Message" and "Outbound Message". For go implementation, if compression is not enabled message-size-compressed is same as actual message. So, i guess what we have as of now is fine? or should we only add if compressed and actual are not equal?
There was a problem hiding this comment.
The text says -
If compression needed, add key message-size-compressed with integer value of compressed message size.
There was a problem hiding this comment.
Thanks. @vinothkumarr227 could you do similar to what Java has https://github.com/grpc/grpc-java/blob/f79ab2f16f7e037da0e8c9985e917eca552c22cb/opentelemetry/src/main/java/io/grpc/opentelemetry/OpenTelemetryTracingModule.java#L439 which is to add message-size-compressed only if compression was enabled.
Do for both inbound and outbound. For compression enabled, check if message-size is not equal to compressed length
Line 89 in 732f3f3
There was a problem hiding this comment.
@purnesh42H I have added both inbound and outbound. For compression to be considered enabled
|
We also need tests for streaming, but that can be added in a subsequent PR |
the e2e tests contains tests for both unary and streaming RPCs. Are you referring to anything else? |
|
@vinothkumarr227 please resolve the conflicts from #8237 |
I have resolved the conflicts |
In all of the tests, I see only a single message being sent/received, which means that the logic on the sequence numbers incremented is untested. |
okay. Filed the issue #8240 |
stats/opentelemetry/trace.go
Outdated
| attribute.Int64("message-size-compressed", int64(rs.CompressedLength)), | ||
| )) | ||
| } | ||
| if rs.CompressedLength > 0 && rs.CompressedLength != rs.Length { |
There was a problem hiding this comment.
nit: no need to check > 0?
stats/opentelemetry/trace.go
Outdated
| attribute.Int64("message-size-compressed", int64(rs.CompressedLength)), | ||
| )) | ||
| } | ||
| if rs.CompressedLength > 0 && rs.CompressedLength != rs.Length { |
There was a problem hiding this comment.
nit: no need to check > 0?
Fixes: #8216
RELEASE NOTES: None