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 81.75182% with 25 lines in your changes missing coverage. Please review.

Project coverage is 82.05%. Comparing base (56a14ba) to head (5571e3b).
Report is 8 commits behind head on master.

Files with missing lines	Patch %	Lines
stats/opentelemetry/trace.go	75.00%	9 Missing and 3 partials ⚠️
stats/opentelemetry/client_metrics.go	85.71%	5 Missing and 2 partials ⚠️
stats/opentelemetry/client_tracing.go	72.72%	2 Missing and 1 partial ⚠️
stats/opentelemetry/server_tracing.go	78.57%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7852      +/-   ##
==========================================
- Coverage   82.09%   82.05%   -0.04%     
==========================================
  Files         379      384       +5     
  Lines       38261    38652     +391     
==========================================
+ Hits        31409    31716     +307     
- Misses       5549     5611      +62     
- Partials     1303     1325      +22

Files with missing lines	Coverage Δ
...elemetry/experimental/grpc_trace_bin_propagator.go	`87.50% <ø> (ø)`
stats/opentelemetry/opentelemetry.go	`76.51% <100.00%> (+0.64%)`	⬆️
stats/opentelemetry/server_metrics.go	`89.82% <100.00%> (+0.44%)`	⬆️
stats/opentelemetry/client_tracing.go	`72.72% <72.72%> (ø)`
stats/opentelemetry/server_tracing.go	`78.57% <78.57%> (ø)`
stats/opentelemetry/client_metrics.go	`86.47% <85.71%> (-1.46%)`	⬇️
stats/opentelemetry/trace.go	`75.00% <75.00%> (ø)`

... and 25 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.

…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.

yeah GRPCTraceBinPropagator is what user can set if they are in the migration path i.e. using opencensus but its not default. The recommendation is to use W3CContextPropagator provided by otel if they are not using opencensus

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 you're right, ideally user should set these variables. I have updated the PR, kindly check.

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 I have updated the PR with the later approach, as it makes sense to me for this to be method of Options struct. Let me know if you think this can be improved.

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?

Updated now, thAnks.

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 added 8 commits December 19, 2024 09:52

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

aranjans added 2 commits December 23, 2024 22:50

refactor and addressed comments from doug

b8fe8db

Add copyright notice to client_tracing.go

d4ae3ab

aranjans assigned dfawley and unassigned aranjans Dec 23, 2024

dfawley assigned aranjans and unassigned dfawley Dec 23, 2024

aranjans assigned dfawley and unassigned aranjans Dec 24, 2024

aranjans force-pushed the a72 branch from 95a523f to de83ba5 Compare December 24, 2024 04:15

let client set the propagator and trace provider

c705b97

aranjans force-pushed the a72 branch from de83ba5 to c705b97 Compare December 24, 2024 04:16

fix breaking tests

5571e3b

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

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

aranjans Dec 23, 2024

dfawley Dec 20, 2024

purnesh42H Dec 23, 2024

dfawley Dec 20, 2024

aranjans Dec 23, 2024

dfawley Dec 20, 2024

dfawley Dec 20, 2024

dfawley Dec 20, 2024

aranjans Dec 23, 2024

dfawley Dec 20, 2024

dfawley Dec 23, 2024

aranjans Dec 24, 2024

dfawley Dec 20, 2024

aranjans Dec 23, 2024

dfawley Dec 20, 2024

aranjans Dec 23, 2024 •

edited

Loading

dfawley Dec 20, 2024

aranjans Dec 23, 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

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

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 Dec 23, 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 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

aranjans Dec 23, 2024 •

edited

Loading