stats/opentelemetry: Introduce Tracing API #7852

aranjans · 2024-11-18T05:20:32Z

This pull request adds the OpenTelemetry tracing API to the grpc-go opentelemetry plugin as outlined in proposal A72.

RELEASE NOTES:

stats/opentelemetry: Introduces an experimental API for enabling and configuring OpenTelemetry tracing within gRPC-go opentelemetry plugin under stats/opentelemetry/experimental. This includes the addition of TraceOptions in the Options struct to allow users to specify the TraceProvider and TextMapPropagator.

codecov · 2024-11-18T05:25:38Z

Codecov Report

Attention: Patch coverage is 85.56701% with 28 lines in your changes missing coverage. Please review.

Project coverage is 82.09%. Comparing base (56a14ba) to head (b6503f7).

Files with missing lines	Patch %	Lines
stats/opentelemetry/trace.go	77.50%	13 Missing and 5 partials ⚠️
stats/opentelemetry/client_metrics.go	83.87%	7 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7852      +/-   ##
==========================================
- Coverage   82.09%   82.09%   -0.01%     
==========================================
  Files         379      380       +1     
  Lines       38261    38405     +144     
==========================================
+ Hits        31409    31527     +118     
- Misses       5549     5568      +19     
- Partials     1303     1310       +7

Files with missing lines	Coverage Δ
clientconn.go	`92.15% <100.00%> (-0.27%)`	⬇️
...elemetry/experimental/grpc_trace_bin_propagator.go	`87.50% <ø> (ø)`
stats/opentelemetry/opentelemetry.go	`76.82% <100.00%> (+0.95%)`	⬆️
stats/opentelemetry/server_metrics.go	`90.17% <100.00%> (+0.79%)`	⬆️
stream.go	`81.72% <100.00%> (+0.03%)`	⬆️
stats/opentelemetry/client_metrics.go	`85.64% <83.87%> (-2.29%)`	⬇️
stats/opentelemetry/trace.go	`77.50% <77.50%> (ø)`

... and 23 files with indirect coverage changes

examples/features/opentelemetry/client/main.go

gcp/observability/go.mod

clientconn.go

interop/xds/go.sum

purnesh42H · 2024-11-20T05:02:06Z

stats/opentelemetry/client_metrics.go

@@ -119,22 +134,46 @@ func (h *clientStatsHandler) streamInterceptor(ctx context.Context, desc *grpc.S
 	}

 	startTime := time.Now()
+	ctx, span := h.createCallTraceSpan(ctx, method)


i think we should just not call createCallTraceSpan if tracing is disabled

stats/opentelemetry/client_metrics.go

stats/opentelemetry/trace.go

stats/opentelemetry/opentelemetry.go

purnesh42H · 2024-11-20T05:24:21Z

@aranjans please update this with latest main branch to get rid of all go.mod and go.sum changes

aranjans · 2024-11-22T01:18:19Z

@purnesh42H Thanks for your review, I have addressed all your comments and this PR is ready for another pass.

purnesh42H

Some more non-test comments. Will review tests in next pass

clientconn.go

stats/handlers.go

stats/opentelemetry/client_metrics.go

stream.go

stats/opentelemetry/client_metrics.go

purnesh42H

Some more non-test comments. Will review tests in next pass

purnesh42H · 2024-11-22T05:36:44Z

@aranjans you should link the gRFC and concise your description

aranjans · 2024-11-23T16:05:05Z

@purnesh42H I have addressed all the comments, and updated the description to link the grfc proposal.
Feel free to close the thread which are resolved now.

stats/opentelemetry/client_metrics.go

stats/opentelemetry/opentelemetry.go

stats/opentelemetry/server_metrics.go

stats/opentelemetry/trace.go

aranjans · 2024-11-30T18:32:36Z

@purnesh42H I have addressed all your comments, and updated the PR. Kindly review the PR.

purnesh42H

lgtm. 3 more documentation comments

purnesh42H · 2024-12-18T17:00:49Z

stats/opentelemetry/e2e_test.go

+// TestMetricsAndTracesOptionEnabled verifies the integration of metrics and traces
+// emitted by the OpenTelemetry instrumentation in a gRPC environment. It sets up a
+// stub server with both metrics and traces enabled, and tests the correct emission
+// of metrics during a Unary RPC and a Streaming RPC. The test ensures that the


nit: correct emission of metrics and traces

purnesh42H · 2024-12-18T17:02:12Z

stats/opentelemetry/e2e_test.go

+	defer cancel()
+
+	// Make two RPC's, a unary RPC and a streaming RPC. These should cause
+	// certain metrics to be emitted, which should be observed through the


this is verifying traces. Please mention traces everywhere in tests which are testing traces and they are observed through span exporter

purnesh42H · 2024-12-18T17:05:23Z

stats/opentelemetry/e2e_test.go

+	defer cancel()
+
+	// Make two RPC's, a unary RPC and a streaming RPC. These should cause
+	// certain metrics to be emitted, which should be observed through the


Same here. We are verifying the traces and verifying using span exporter

purnesh42H · 2024-12-18T17:08:49Z

stats/opentelemetry/e2e_test.go

+// of metrics during a Unary RPC and a Streaming RPC. The test ensures that the
+// emitted metrics reflect the operations performed, including the size of the
+// compressed message, and verifies that tracing information is correctly recorded.
+func (s) TestMetricsAndTracesOptionEnabled(t *testing.T) {


@aranjans may be one more tests to verify metrics and traces are not emitted if they are not enabled?

@purnesh42H I have added the test. Please note: I have added the test which verifies the current setup works as expected when traces and metrics are disabled.

purnesh42H · 2024-12-18T17:12:09Z

@aranjans looks like there is some segmentation fault in stats package https://github.com/grpc/grpc-go/actions/runs/12372209143/job/34529917692?pr=7852. It looks like some test is written wrong

…y_test to opentelemetry

dfawley · 2024-12-19T18:17:24Z

stream.go

@@ -213,13 +213,13 @@ func newClientStream(ctx context.Context, desc *StreamDesc, cc *ClientConn, meth
 	// Provide an opportunity for the first RPC to see the first service config
 	// provided by the resolver.
 	if err := cc.waitForResolvedAddrs(ctx); err != nil {
+		cc.nameResolutionDelayed = true


What is this trying to capture? I don't think this is what you want.

For example, if name resolution takes 30 seconds to occur, and there is no deadline on the RPC, then this will not ever be set.

Also, making this a boolean field on the ClientConn can't be correct.

Its for capturing the delay in name resolution https://github.com/grpc/proposal/blob/master/A72-open-telemetry-tracing.md#tracing-information

I think we need to specifically look for context timeout error to set this? waitForResolvedAddrs doesn't directly calculate delay; it waits for resolved addresses, and if resolution takes too long, it will return a timeout error?

What is the expected behavior when there is no deadline?

The goal of the feature is to determine, if a name resolution delay occurred during the RPC's lifespan, how long that took.

So basically you want to add events to the lifecycle of the RPC when these things occur:

grpc-go/clientconn.go

Lines 682 to 684 in e957825

select {

case <-cc.firstResolveEvent.Done():

return nil

Thanks @dfawley and @purnesh42H for your thoughts, I will create a separate PR for this as a bonus feature.

aranjans · 2024-12-20T18:07:52Z

@dfawley As per our offline discussion to exclude changes of adding event for name resolution delay, and create a separate PR. I have updated the PR.

dfawley · 2024-12-20T18:23:52Z

stream.go

@@ -215,7 +215,6 @@ func newClientStream(ctx context.Context, desc *StreamDesc, cc *ClientConn, meth
 	if err := cc.waitForResolvedAddrs(ctx); err != nil {
 		return nil, err
 	}
-


Please just revert this file.

dfawley · 2024-12-20T20:51:03Z

stats/opentelemetry/experimental/trace_options.go

+	// TextMapPropagator propagates span context through text map carrier.
+	TextMapPropagator propagation.TextMapPropagator


Is the user supposed to be able to override this?

We have a default for them, though, right? That should get documented.

dfawley · 2024-12-20T20:51:53Z

stats/opentelemetry/experimental/trace_options.go

Should this go into experimental/opentelemetry, or even experimental/stats/opentelemetry? Either way there should be comments indicating that it will be moved (and where it will be moved to, and when).

dfawley · 2024-12-20T20:52:48Z

stats/opentelemetry/opentelemetry.go

+func init() {
+	otelinternal.SetPluginOption = func(o *Options, po otelinternal.PluginOption) {
+		o.MetricsOptions.pluginOption = po
+	}
+}
+


Can you move this back to where it was to avoid the code churn please?

dfawley · 2024-12-20T20:53:22Z

stats/opentelemetry/server_metrics.go

@@ -66,6 +68,15 @@ func (h *serverStatsHandler) initializeMetrics() {
 	rm.registerMetrics(metrics, meter)
 }

+func (h *serverStatsHandler) initializeTracing() {


Should this go into a new file? server_tracing.go?

dfawley · 2024-12-20T21:02:37Z

stats/opentelemetry/opentelemetry.go

@@ -171,6 +176,14 @@ func removeLeadingSlash(mn string) string {
 	return strings.TrimLeft(mn, "/")
 }

+func isMetricsDisabled(mo MetricsOptions) bool {


The code now looks like:

if !isMetricsDisabled() { }

It should instead be:

if isMetricsEnabled() { }

to avoid a double negative.

dfawley · 2024-12-20T21:03:59Z

stats/opentelemetry/server_metrics.go

+	otel.SetTextMapPropagator(h.options.TraceOptions.TextMapPropagator)
+	otel.SetTracerProvider(h.options.TraceOptions.TracerProvider)


IIUC, these are global settings. I don't think that's what we want... Only the user should ever be calling these kinds of function, not our library.

dfawley · 2024-12-20T21:06:03Z

stats/opentelemetry/opentelemetry.go

+	return mo.MeterProvider == nil
+}
+
+func isTracingDisabled(to experimental.TraceOptions) bool {


We should discuss options, but requiring these to be called multiple times from the stats handler feels problematic. Maybe there's a tracing stats handler and a metrics stats handler and we combine them based on which ones are enabled?

At the very least, these should become methods so we don't have to keep reaching into our data structures when calling them to find the options fields.

dfawley · 2024-12-20T21:07:31Z

stats/opentelemetry/client_metrics.go

 	}
-	ri := &rpcInfo{
+	ai.startTime = startTime


Why did you change how this is set?

Why are you setting a local 3 lines above to time.Now() and then assigning this field to that?

dfawley · 2024-12-20T21:08:20Z

stats/opentelemetry/server_metrics.go

+	ai := &attemptInfo{}
+	startTime := time.Now()


Similar comments as above.

aranjans added this to the 1.69 Release milestone Nov 18, 2024

aranjans added Type: Feature New features or improvements in behavior Area: Observability Includes Stats, Tracing, Channelz, Healthz, Binlog, Reflection, Admin, GCP Observability labels Nov 18, 2024

aranjans requested a review from purnesh42H November 18, 2024 08:22

purnesh42H self-assigned this Nov 19, 2024

purnesh42H reviewed Nov 20, 2024

View reviewed changes

purnesh42H assigned aranjans and unassigned purnesh42H Nov 20, 2024

aranjans force-pushed the a72 branch 3 times, most recently from 378ddd3 to 8cb8222 Compare November 21, 2024 17:44

aranjans assigned purnesh42H and unassigned aranjans Nov 22, 2024

purnesh42H reviewed Nov 22, 2024

View reviewed changes

purnesh42H assigned aranjans and unassigned purnesh42H Nov 22, 2024

purnesh42H changed the title ~~Implement A72: OpenTelemetry Tracing~~ stats/opentelemetry: Introduce Tracing API Nov 22, 2024

aranjans assigned purnesh42H and unassigned aranjans Nov 23, 2024

purnesh42H reviewed Nov 25, 2024

View reviewed changes

purnesh42H assigned aranjans and unassigned purnesh42H Nov 25, 2024

aranjans force-pushed the a72 branch from f50a6b9 to ce57921 Compare November 30, 2024 18:29

aranjans removed their assignment Dec 18, 2024

purnesh42H approved these changes Dec 18, 2024

View reviewed changes

purnesh42H reviewed Dec 18, 2024

View reviewed changes

purnesh42H self-requested a review December 18, 2024 17:10

purnesh42H requested a review from dfawley December 19, 2024 03:28

aranjans added 14 commits December 19, 2024 09:52

final rebase with master

fc09e2c

update e2e tests

2747371

fix e2e tests

96f92a0

remove fmt logs

f650e37

move grpc_trace_bin_propagator to experimental

175d4c5

move TraceOptions api to experimental

c30c44a

addressed purnesh's comments

90ffa23

don't change opentelemetry/e2e_test.go package name from opentelemetr…

2083830

…y_test to opentelemetry

make vet happy

e8e9d53

update TestServerWithMetricsAndTraceOptions

57fd38a

fixed nits

b0aad8a

fixed nits

8680ae8

fix: small nits

98d11b7

Add test with metrics and traces disabled.

d0e1a0a

aranjans force-pushed the a72 branch from b93666d to d0e1a0a Compare December 19, 2024 04:22

make vet happy

b6503f7

dfawley reviewed Dec 19, 2024

View reviewed changes

pull out logic to find name resolution delay

b2831f1

remove experimental notice from grpc_trace_bin_propagator

1cb4396

aranjans unassigned purnesh42H Dec 20, 2024

dfawley reviewed Dec 20, 2024

View reviewed changes

dfawley assigned aranjans and unassigned dfawley Dec 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stats/opentelemetry: Introduce Tracing API #7852

stats/opentelemetry: Introduce Tracing API #7852

aranjans commented Nov 18, 2024 •

edited by purnesh42H

Loading

codecov bot commented Nov 18, 2024 •

edited

Loading

purnesh42H Nov 20, 2024

aranjans Nov 20, 2024 •

edited

Loading

purnesh42H commented Nov 20, 2024

aranjans commented Nov 22, 2024

purnesh42H left a comment

purnesh42H left a comment

purnesh42H commented Nov 22, 2024

aranjans commented Nov 23, 2024 •

edited

Loading

aranjans commented Nov 30, 2024

purnesh42H left a comment

purnesh42H Dec 18, 2024

purnesh42H Dec 18, 2024

purnesh42H Dec 18, 2024

purnesh42H Dec 18, 2024

aranjans Dec 19, 2024

purnesh42H commented Dec 18, 2024

dfawley Dec 19, 2024

purnesh42H Dec 20, 2024 •

edited

Loading

dfawley Dec 20, 2024

aranjans Dec 20, 2024

aranjans commented Dec 20, 2024

dfawley Dec 20, 2024

dfawley Dec 20, 2024

dfawley Dec 20, 2024

dfawley Dec 20, 2024

dfawley Dec 20, 2024

dfawley Dec 20, 2024

dfawley Dec 20, 2024

dfawley Dec 20, 2024

dfawley Dec 20, 2024

dfawley Dec 20, 2024

		// TextMapPropagator propagates span context through text map carrier.
		TextMapPropagator propagation.TextMapPropagator

		otel.SetTextMapPropagator(h.options.TraceOptions.TextMapPropagator)
		otel.SetTracerProvider(h.options.TraceOptions.TracerProvider)

stats/opentelemetry: Introduce Tracing API #7852

Are you sure you want to change the base?

stats/opentelemetry: Introduce Tracing API #7852

Conversation

aranjans commented Nov 18, 2024 • edited by purnesh42H Loading

codecov bot commented Nov 18, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

aranjans Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

purnesh42H commented Nov 20, 2024

aranjans commented Nov 22, 2024

purnesh42H left a comment

Choose a reason for hiding this comment

purnesh42H left a comment

Choose a reason for hiding this comment

purnesh42H commented Nov 22, 2024

aranjans commented Nov 23, 2024 • edited Loading

aranjans commented Nov 30, 2024

purnesh42H left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purnesh42H commented Dec 18, 2024

Choose a reason for hiding this comment

purnesh42H Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aranjans commented Dec 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aranjans commented Nov 18, 2024 •

edited by purnesh42H

Loading

codecov bot commented Nov 18, 2024 •

edited

Loading

aranjans Nov 20, 2024 •

edited

Loading

aranjans commented Nov 23, 2024 •

edited

Loading

purnesh42H Dec 20, 2024 •

edited

Loading