Aquileo | stats/opentelemetry: Introduce Tracing API by aranjans · Pull Request #7852 · grpc/grpc-go

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

Project coverage is 82.25%. Comparing base (56a14ba) to head (374b933).
Report is 46 commits behind head on master.

Files with missing lines	Patch %	Lines
stats/opentelemetry/trace.go	83.33%	7 Missing and 1 partial ⚠️
stats/opentelemetry/client_tracing.go	76.47%	3 Missing and 1 partial ⚠️
stats/opentelemetry/opentelemetry.go	62.50%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7852      +/-   ##
==========================================
+ Coverage   82.09%   82.25%   +0.16%     
==========================================
  Files         379      387       +8     
  Lines       38261    39041     +780     
==========================================
+ Hits        31409    32114     +705     
- Misses       5549     5595      +46     
- Partials     1303     1332      +29

Files with missing lines	Coverage Δ
stats/opentelemetry/client_metrics.go	`89.39% <100.00%> (+1.46%)`	⬆️
stats/opentelemetry/server_metrics.go	`89.82% <100.00%> (+0.44%)`	⬆️
stats/opentelemetry/server_tracing.go	`100.00% <100.00%> (ø)`
stats/opentelemetry/opentelemetry.go	`75.00% <62.50%> (-0.87%)`	⬇️
stats/opentelemetry/client_tracing.go	`76.47% <76.47%> (ø)`
stats/opentelemetry/trace.go	`83.33% <83.33%> (ø)`

... and 59 files with indirect coverage changes

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

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.

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

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

dfawley · 2025-01-09T21:40:01Z

+ * limitations under the License.
+ */
+
+package experimental


The directory name is opentelemetry so the package name should match.

This package should clearly indicate that it will be removed, with the contents moved to stats/opentelemetry, and should also indicate when that will happen.

dfawley · 2025-01-09T21:47:47Z

+	}
 	err := invoker(ctx, method, req, reply, cc, opts...)
-	h.perCallMetrics(ctx, err, startTime, ci)
+	h.perCallTracesAndMetrics(ctx, err, startTime, ci, span)


@purnesh42H

How much code & data is shared between the tracing and metrics implementations? I don't believe they share almost anything?

Would it make sense to implement a separate set of interceptors for tracing & metrics so that we can avoid checking to see which one is implemented for every event in the life of an RPC? I think the code would end up simpler/cleaner/easier to review that way, too.

@dfawley they only share the DialOption and ServerOption as per proposal. Are you suggesting to have 4 interceptors internally? Both stream and unary each for metrics and traces? If/When we add logging, we will add 2 more then. But yeah we will have separate path for each and then don't have to check if the other one is implemented as well.

Yes, I think it would helpful to have separate handlers for the different functionality, since they share nothing in common and can be enabled/disabled independently.

dfawley · 2025-01-09T21:49:54Z

+	if h.options.isTracingEnabled() && ts != nil {
+		s := status.Convert(err)
+		if s.Code() == grpccodes.OK {
+			(*ts).SetStatus(otelcodes.Ok, s.Message())


Does ts.SetStatus not work? Go should auto-deref.

No, it does not auto dereference them.

Oh, that's because trace.Span is an interface. Why are you passing it by pointer? That seems wrong.

dfawley · 2025-01-09T21:52:03Z

+		return ctx, nil
+	}
+
+	mn := "Attempt." + strings.Replace(removeLeadingSlash(rti.FullMethodName), "/", ".", -1)


attemptInfo appears to already have the method name without the leading slash. Use that one instead?

dfawley · 2025-01-13T23:59:14Z

+	if h.options.isTracingEnabled() && ts != nil {
+		s := status.Convert(err)
+		if s.Code() == grpccodes.OK {
+			(*ts).SetStatus(otelcodes.Ok, s.Message())


Oh, that's because trace.Span is an interface. Why are you passing it by pointer? That seems wrong.

dfawley · 2025-01-14T00:01:35Z

+// span into gRPC Metadata, if TextMapPropagator is provided in the trace
+// options. if TextMapPropagator is not provided, it returns the context as is.
+func (h *clientStatsHandler) traceTagRPC(ctx context.Context, rti *stats.RPCTagInfo, ai *attemptInfo) (context.Context, *attemptInfo) {
+	mn := "Attempt." + strings.Replace(removeLeadingSlash(ai.method), "/", ".", -1)


ai.method already had the leading slash removed, so remove the call to attempt it again?

Oh wait.. it looks like it's removed in the server stats handler but not the client stats handler?

Is that intentional or just a mistake?

Ah I see, it got missed. I have corrected it now, PTAL.

dfawley · 2025-01-14T00:04:36Z

+		// message id - "must be calculated as two different counters starting
+		// from one for sent messages and one for received messages."
+		mi := ai.countRecvMsg + 1
+		ai.countRecvMsg = ai.countRecvMsg + 1


ai.countRecvMsg++ ?

dfawley · 2025-01-14T00:04:48Z

+	case *stats.InPayload:
+		// message id - "must be calculated as two different counters starting
+		// from one for sent messages and one for received messages."
+		mi := ai.countRecvMsg + 1


Why? Just read ai.countRecvMsg later directly?

dfawley · 2025-01-14T00:04:59Z

+		mi := ai.countSentMsg + 1
+		ai.countSentMsg = ai.countSentMsg + 1


…r is not

aranjans · 2025-01-15T11:53:21Z

@dfawley I have addressed all your comments including the nits. PTAL.

purnesh42H · 2025-01-15T15:57:13Z

+}
+
+func (o *Options) isTracingEnabled() bool {
+	return o.TraceOptions.TextMapPropagator != nil


@dfawley @aranjans did we decide to remove checking for TracerProvider? Both are mandatory.

This is the same topic as this other comment thread, right?:

#7852 (comment)

dfawley

Just a few more small things otherwise LGTM

dfawley · 2025-01-23T18:27:53Z

+	case *stats.End:
+		if rs.Error != nil {
+			s := status.Convert(rs.Error)
+			span.SetStatus(otelcodes.Error, s.Message())


It seems there is missing coverage for this (RPC error) and PickerUpdated - could you add test cases for them, please?

@dfawley I have added the test for rpc error, PTAL.

dfawley · 2025-01-24T21:00:22Z

 	}

+	// Log an error if one of the options is missing.
+	if (h.options.TraceOptions.TextMapPropagator == nil) != (h.options.TraceOptions.TracerProvider == nil) {


This check should happen in the constructor for the otel plugin. Not on every RPC.

purnesh42H · 2025-01-28T08:17:50Z

 		o.MetricsOptions.pluginOption = po
+		// Log an error if one of the options is missing.
+		if (o.TraceOptions.TextMapPropagator == nil) != (o.TraceOptions.TracerProvider == nil) {
+			logger.Error("traceOptions are not set properly: one of TextMapPropagator or TracerProvider is missing.")


@aranjans How about "Tracing will not be recorded because traceOptions are not set properly: one of TextMapPropagator or TracerProvider is missing"? Also, may be it should be warning instead of error because we are only treating it as no-op for tracing but continuing with setup? @dfawley cc

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

purnesh42H requested a review from dfawley January 9, 2025 05:10

dfawley reviewed Jan 9, 2025

View reviewed changes

dfawley assigned purnesh42H and aranjans and unassigned dfawley Jan 9, 2025

move grpc_trace_bin_propagator -> stats/opentelemetry

cfb92ae

aranjans assigned dfawley and purnesh42H and unassigned purnesh42H and aranjans Jan 13, 2025

dfawley reviewed Jan 14, 2025

View reviewed changes

dfawley assigned aranjans and unassigned dfawley Jan 14, 2025

aranjans removed their assignment Jan 14, 2025

aranjans added 2 commits January 15, 2025 17:02

nits

72e178b

disable tracing if textMapPropagator is not set, even if traceProvide…

5beafe1

…r is not

purnesh42H reviewed Jan 15, 2025

View reviewed changes

aranjans added 2 commits January 21, 2025 21:56

return noop interceptor if traceoptions are not defined properly

8e15369

make vet happy

d5913b6

dfawley reviewed Jan 23, 2025

View reviewed changes

fix nits

afbf947

dfawley reviewed Jan 24, 2025

View reviewed changes

aranjans added 2 commits January 28, 2025 12:06

add test for rpc error

2fc523f

make vet happy

c1569fc

purnesh42H reviewed Jan 28, 2025

View reviewed changes

Comment thread stats/opentelemetry/client_metrics.go Outdated

purnesh42H reviewed Jan 28, 2025

View reviewed changes

address purnesh comments

374b933

dfawley approved these changes Jan 30, 2025

View reviewed changes

arjan-bal mentioned this pull request Jan 30, 2025

Flaky Test: MetricsAndTracesOptionEnabled #8053

Closed

		mi := ai.countSentMsg + 1
		ai.countSentMsg = ai.countSentMsg + 1

Conversation

aranjans commented Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

purnesh42H commented Nov 20, 2024

Uh oh!

aranjans commented Nov 22, 2024

Uh oh!

purnesh42H left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

purnesh42H left a comment

Choose a reason for hiding this comment

Uh oh!

purnesh42H commented Nov 22, 2024

Uh oh!

aranjans commented Nov 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aranjans commented Nov 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aranjans commented Nov 18, 2024 •
edited

Loading

codecov Bot commented Nov 18, 2024 •
edited

Loading

aranjans commented Nov 23, 2024 •
edited

Loading

purnesh42H Jan 28, 2025 •
edited

Loading