Skip to content

New A72 changes for OpenTelemetry #8216 #8226

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
purnesh42H merged 8 commits into from
Apr 17, 2025
Merged

New A72 changes for OpenTelemetry #8216 #8226

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ad07d2d
Fixed the trace event cleanup
vinothkumarr227 Apr 4, 2025
01927ac
Fixed the test cases
vinothkumarr227 Apr 4, 2025
8cd9b0b
Fixed the trace cleanup
vinothkumarr227 Apr 4, 2025
681ce72
small tweaks
vinothkumarr227 Apr 8, 2025
64124ff
Merge branch 'master' into a72-trace-cleanup
vinothkumarr227 Apr 9, 2025
6886a15
small tweaks
vinothkumarr227 Apr 9, 2025
ed45ee7
trace: add message-size-compressed only when compression is enabled
vinothkumarr227 Apr 10, 2025
9a05b61
small tweaks
vinothkumarr227 Apr 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion stats/opentelemetry/client_tracing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ func (h *clientStatsHandler) createCallTraceSpan(ctx context.Context, method str
logger.Error("TraceProvider is not provided in trace options")
return ctx, nil
}
mn := strings.Replace(removeLeadingSlash(method), "/", ".", -1)
mn := "Sent." + strings.Replace(removeLeadingSlash(method), "/", ".", -1)
tracer := h.options.TraceOptions.TracerProvider.Tracer(tracerName, trace.WithInstrumentationVersion(grpc.Version))
ctx, span := tracer.Start(ctx, mn, trace.WithSpanKind(trace.SpanKindClient))
return ctx, span
Expand Down
67 changes: 37 additions & 30 deletions stats/opentelemetry/e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,21 +204,28 @@ func validateTraces(t *testing.T, spans tracetest.SpanStubs, wantSpanInfos []tra

for _, span := range spans {
switch {
case span.Name == "grpc.testing.TestService.UnaryCall":
case span.Name == "Sent.grpc.testing.TestService.UnaryCall":
isUnary = true
if span.SpanKind == oteltrace.SpanKindClient {
unaryClient = &span
} else {
}
case span.Name == "Recv.grpc.testing.TestService.UnaryCall":
isUnary = true
if span.SpanKind == oteltrace.SpanKindServer {
unaryServer = &span
}
case span.Name == "Attempt.grpc.testing.TestService.UnaryCall":
isUnary = true
unaryAttempt = &span
case span.Name == "grpc.testing.TestService.FullDuplexCall":

case span.Name == "Sent.grpc.testing.TestService.FullDuplexCall":
isStream = true
if span.SpanKind == oteltrace.SpanKindClient {
streamClient = &span
} else {
}
case span.Name == "Recv.grpc.testing.TestService.FullDuplexCall":
isStream = true
if span.SpanKind == oteltrace.SpanKindServer {
streamServer = &span
}
case span.Name == "Attempt.grpc.testing.TestService.FullDuplexCall":
Expand Down Expand Up @@ -850,7 +857,7 @@ func (s) TestMetricsAndTracesOptionEnabled(t *testing.T) {

wantSpanInfos := []traceSpanInfo{
{
name: "grpc.testing.TestService.UnaryCall",
name: "Recv.grpc.testing.TestService.UnaryCall",
spanKind: oteltrace.SpanKindServer.String(),
attributes: []attribute.KeyValue{
{
Expand All @@ -872,7 +879,7 @@ func (s) TestMetricsAndTracesOptionEnabled(t *testing.T) {
},
events: []trace.Event{
{
Name: "Inbound compressed message",
Name: "Inbound message",
Attributes: []attribute.KeyValue{
{
Key: "sequence-number",
Expand All @@ -889,7 +896,7 @@ func (s) TestMetricsAndTracesOptionEnabled(t *testing.T) {
},
},
{
Name: "Outbound compressed message",
Name: "Outbound message",
Attributes: []attribute.KeyValue{
{
Key: "sequence-number",
Expand Down Expand Up @@ -930,7 +937,7 @@ func (s) TestMetricsAndTracesOptionEnabled(t *testing.T) {
},
events: []trace.Event{
{
Name: "Outbound compressed message",
Name: "Outbound message",
Attributes: []attribute.KeyValue{
{
Key: "sequence-number",
Expand All @@ -947,7 +954,7 @@ func (s) TestMetricsAndTracesOptionEnabled(t *testing.T) {
},
},
{
Name: "Inbound compressed message",
Name: "Inbound message",
Attributes: []attribute.KeyValue{
{
Key: "sequence-number",
Expand All @@ -966,13 +973,13 @@ func (s) TestMetricsAndTracesOptionEnabled(t *testing.T) {
},
},
{
name: "grpc.testing.TestService.UnaryCall",
name: "Sent.grpc.testing.TestService.UnaryCall",
spanKind: oteltrace.SpanKindClient.String(),
attributes: nil,
events: nil,
},
{
name: "grpc.testing.TestService.FullDuplexCall",
name: "Recv.grpc.testing.TestService.FullDuplexCall",
spanKind: oteltrace.SpanKindServer.String(),
attributes: []attribute.KeyValue{
{
Expand All @@ -995,7 +1002,7 @@ func (s) TestMetricsAndTracesOptionEnabled(t *testing.T) {
events: nil,
},
{
name: "grpc.testing.TestService.FullDuplexCall",
name: "Sent.grpc.testing.TestService.FullDuplexCall",
spanKind: oteltrace.SpanKindClient.String(),
attributes: nil,
events: nil,
Expand Down Expand Up @@ -1074,7 +1081,7 @@ func (s) TestSpan(t *testing.T) {

wantSpanInfos := []traceSpanInfo{
{
name: "grpc.testing.TestService.UnaryCall",
name: "Recv.grpc.testing.TestService.UnaryCall",
spanKind: oteltrace.SpanKindServer.String(),
attributes: []attribute.KeyValue{
{
Expand All @@ -1096,7 +1103,7 @@ func (s) TestSpan(t *testing.T) {
},
events: []trace.Event{
{
Name: "Inbound compressed message",
Name: "Inbound message",
Attributes: []attribute.KeyValue{
{
Key: "sequence-number",
Expand All @@ -1113,7 +1120,7 @@ func (s) TestSpan(t *testing.T) {
},
},
{
Name: "Outbound compressed message",
Name: "Outbound message",
Attributes: []attribute.KeyValue{
{
Key: "sequence-number",
Expand Down Expand Up @@ -1154,7 +1161,7 @@ func (s) TestSpan(t *testing.T) {
},
events: []trace.Event{
{
Name: "Outbound compressed message",
Name: "Outbound message",
Attributes: []attribute.KeyValue{
{
Key: "sequence-number",
Expand All @@ -1171,7 +1178,7 @@ func (s) TestSpan(t *testing.T) {
},
},
{
Name: "Inbound compressed message",
Name: "Inbound message",
Attributes: []attribute.KeyValue{
{
Key: "sequence-number",
Expand All @@ -1190,13 +1197,13 @@ func (s) TestSpan(t *testing.T) {
},
},
{
name: "grpc.testing.TestService.UnaryCall",
name: "Sent.grpc.testing.TestService.UnaryCall",
spanKind: oteltrace.SpanKindClient.String(),
attributes: nil,
events: nil,
},
{
name: "grpc.testing.TestService.FullDuplexCall",
name: "Recv.grpc.testing.TestService.FullDuplexCall",
spanKind: oteltrace.SpanKindServer.String(),
attributes: []attribute.KeyValue{
{
Expand All @@ -1219,7 +1226,7 @@ func (s) TestSpan(t *testing.T) {
events: nil,
},
{
name: "grpc.testing.TestService.FullDuplexCall",
name: "Sent.grpc.testing.TestService.FullDuplexCall",
spanKind: oteltrace.SpanKindClient.String(),
attributes: nil,
events: nil,
Expand Down Expand Up @@ -1300,7 +1307,7 @@ func (s) TestSpan_WithW3CContextPropagator(t *testing.T) {

wantSpanInfos := []traceSpanInfo{
{
name: "grpc.testing.TestService.UnaryCall",
name: "Recv.grpc.testing.TestService.UnaryCall",
spanKind: oteltrace.SpanKindServer.String(),
attributes: []attribute.KeyValue{
{
Expand All @@ -1322,7 +1329,7 @@ func (s) TestSpan_WithW3CContextPropagator(t *testing.T) {
},
events: []trace.Event{
{
Name: "Inbound compressed message",
Name: "Inbound message",
Attributes: []attribute.KeyValue{
{
Key: "sequence-number",
Expand All @@ -1339,7 +1346,7 @@ func (s) TestSpan_WithW3CContextPropagator(t *testing.T) {
},
},
{
Name: "Outbound compressed message",
Name: "Outbound message",
Attributes: []attribute.KeyValue{
{
Key: "sequence-number",
Expand Down Expand Up @@ -1380,7 +1387,7 @@ func (s) TestSpan_WithW3CContextPropagator(t *testing.T) {
},
events: []trace.Event{
{
Name: "Outbound compressed message",
Name: "Outbound message",
Attributes: []attribute.KeyValue{
{
Key: "sequence-number",
Expand All @@ -1397,7 +1404,7 @@ func (s) TestSpan_WithW3CContextPropagator(t *testing.T) {
},
},
{
Name: "Inbound compressed message",
Name: "Inbound message",
Attributes: []attribute.KeyValue{
{
Key: "sequence-number",
Expand All @@ -1416,13 +1423,13 @@ func (s) TestSpan_WithW3CContextPropagator(t *testing.T) {
},
},
{
name: "grpc.testing.TestService.UnaryCall",
name: "Sent.grpc.testing.TestService.UnaryCall",
spanKind: oteltrace.SpanKindClient.String(),
attributes: nil,
events: nil,
},
{
name: "grpc.testing.TestService.FullDuplexCall",
name: "Recv.grpc.testing.TestService.FullDuplexCall",
spanKind: oteltrace.SpanKindServer.String(),
attributes: []attribute.KeyValue{
{
Expand All @@ -1445,7 +1452,7 @@ func (s) TestSpan_WithW3CContextPropagator(t *testing.T) {
events: nil,
},
{
name: "grpc.testing.TestService.FullDuplexCall",
name: "Sent.grpc.testing.TestService.FullDuplexCall",
spanKind: oteltrace.SpanKindClient.String(),
attributes: nil,
events: nil,
Expand Down Expand Up @@ -1601,7 +1608,7 @@ func (s) TestTraceSpan_WithRetriesAndNameResolutionDelay(t *testing.T) {
_, err := client.UnaryCall(ctx, &testpb.SimpleRequest{})
return err
},
spanName: "grpc.testing.TestService.UnaryCall",
spanName: "Sent.grpc.testing.TestService.UnaryCall",
},
{
name: "streaming",
Expand Down Expand Up @@ -1645,7 +1652,7 @@ func (s) TestTraceSpan_WithRetriesAndNameResolutionDelay(t *testing.T) {
}
return nil
},
spanName: "grpc.testing.TestService.FullDuplexCall",
spanName: "Sent.grpc.testing.TestService.FullDuplexCall",
},
}

Expand Down
2 changes: 1 addition & 1 deletion stats/opentelemetry/server_tracing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import (
// If TextMapPropagator is not provided in the trace options, it returns context
// as is.
func (h *serverStatsHandler) traceTagRPC(ctx context.Context, ai *attemptInfo) (context.Context, *attemptInfo) {
mn := strings.Replace(ai.method, "/", ".", -1)
mn := "Recv." + strings.Replace(ai.method, "/", ".", -1)
var span trace.Span
tracer := h.options.TraceOptions.TracerProvider.Tracer(tracerName, trace.WithInstrumentationVersion(grpc.Version))
ctx = h.options.TraceOptions.TextMapPropagator.Extract(ctx, otelinternaltracing.NewIncomingCarrier(ctx))
Expand Down
8 changes: 6 additions & 2 deletions stats/opentelemetry/trace.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@
* limitations under the License.
*/

// OpenCensus's binary format for grpc-trace-bin:
Copy link
Contributor

@purnesh42H purnesh42H Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to add this. This was only for grfc

Copy link
Contributor Author

@vinothkumarr227 vinothkumarr227 Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

// https://github.com/census-instrumentation/opencensus-specs/blob/master/
// encodings/BinaryEncoding.md

package opentelemetry

import (
Expand Down Expand Up @@ -58,14 +62,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)),
Copy link
Member

@yashykt yashykt Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the grfc says to only add this if the message is actually compressed

Copy link
Contributor

@purnesh42H purnesh42H Apr 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@yashykt yashykt Apr 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The text says -
If compression needed, add key message-size-compressed with integer value of compressed message size.

Copy link
Contributor

@purnesh42H purnesh42H Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

grpc-go/stats/stats.go

Line 89 in 732f3f3

// CompressedLength is the size of the compressed payload data. Does not

Copy link
Contributor Author

@vinothkumarr227 vinothkumarr227 Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@purnesh42H I have added both inbound and outbound. For compression to be considered enabled

))
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)),
Copy link
Member

@yashykt yashykt Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the grfc says to only add this if the message is actually compressed

Expand Down