stats/opentelemetry: separate out interceptors for tracing and metrics #8063

janardhanvissa · 2025-02-03T11:09:00Z

RELEASE NOTE: None

codecov · 2025-02-03T11:13:43Z

Codecov Report

Attention: Patch coverage is 68.99225% with 40 lines in your changes missing coverage. Please review.

Project coverage is 82.30%. Comparing base (5668c66) to head (2a914e3).

Files with missing lines	Patch %	Lines
stats/opentelemetry/client_tracing.go	58.82%	23 Missing and 5 partials ⚠️
stats/opentelemetry/server_tracing.go	60.00%	9 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8063      +/-   ##
==========================================
- Coverage   82.32%   82.30%   -0.03%     
==========================================
  Files         392      392              
  Lines       39140    39207      +67     
==========================================
+ Hits        32222    32269      +47     
- Misses       5597     5611      +14     
- Partials     1321     1327       +6

Files with missing lines	Coverage Δ
stats/opentelemetry/client_metrics.go	`87.79% <100.00%> (-1.61%)`	⬇️
stats/opentelemetry/opentelemetry.go	`76.82% <100.00%> (+1.82%)`	⬆️
stats/opentelemetry/server_metrics.go	`89.30% <100.00%> (-0.52%)`	⬇️
stats/opentelemetry/server_tracing.go	`68.42% <60.00%> (-31.58%)`	⬇️
stats/opentelemetry/client_tracing.go	`59.49% <58.82%> (-16.98%)`	⬇️

... and 18 files with indirect coverage changes

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

purnesh42H · 2025-02-05T19:15:18Z

@janardhanvissa its not clear what is the intention of this refactor. The follow up from opentelemetry tracing API PR was to create separate interceptors for metrics and traces. Right now, single interceptor is handling both trace and metrics options. Once we have separate unary and stream interceptor each for tracing and metrics, we don't have to check for options disabled/enabled everytime.

purnesh42H

Please see this discussion https://github.com/grpc/grpc-go/pull/7852/files#r1909469701 and modify accordingly.

purnesh42H · 2025-02-19T06:46:08Z

stats/opentelemetry/client_metrics.go

+type clientMetricsStatsHandler struct {
+	*clientStatsHandler
+}
+type clientTracingStatsHandler struct {


new line in between struct declaration

purnesh42H · 2025-02-19T06:49:16Z

stats/opentelemetry/client_metrics.go

+		method: h.determineMethod(method, opts...),
+	}
+	ctx = setCallInfo(ctx, ci)
+	if h.options.MetricsOptions.pluginOption != nil {


this metadata part is not applicable for tracing. It should only be in metrics interceptor. Please remove

purnesh42H · 2025-02-19T06:49:52Z

stats/opentelemetry/client_metrics.go

+	}
+	ctx = setCallInfo(ctx, ci)
+
+	if h.options.MetricsOptions.pluginOption != nil {


same. this metadata part is not applicable for tracing. It should only be in metrics interceptor. Please remove

purnesh42H · 2025-02-19T06:52:00Z

stats/opentelemetry/client_metrics.go

+}
+
+// perCallTraces records per call trace spans.
+func (h *clientTracingStatsHandler) perCallTraces(_ context.Context, err error, _ time.Time, _ *callInfo, ts trace.Span) {


since we don't need time and callInfo, we should not just have them here instead of replacing them with _

stats/opentelemetry/opentelemetry.go

purnesh42H · 2025-02-19T06:59:12Z

stats/opentelemetry/server_metrics.go

@@ -211,24 +218,41 @@ func (h *serverStatsHandler) TagRPC(ctx context.Context, info *stats.RPCTagInfo)

 // HandleRPC implements per RPC tracing and stats implementation.
 func (h *serverStatsHandler) HandleRPC(ctx context.Context, rs stats.RPCStats) {
+	if h.options.isMetricsEnabled() {


we should have separate HandleRPC for metrics and trace statsHandlers instead of having common which requires to check what is enabled etc.

purnesh42H · 2025-02-21T09:17:31Z

stats/opentelemetry/client_metrics.go

+}
+
+type clientTracingStatsHandler struct {
+	*clientStatsHandler


why are we extending clientStatsHandler. Let's just keep all stats handler embedding estats.MetricsRecorder. Increasing levels in hierarchy doesn't help in anyway because metrics and traces stats handler doesn't share common functionality.

I think keeping the inheritance (clientMetricsStatsHandler and clientTracingStatsHandler embedding *clientStatsHandler) would be beneficial because it avoids duplicating the common fields and methods in both metrics and tracing handlers.

As far as i know there is nothing common between metrics and traces so its not providing any help. Which are the common things you found?

Exactly, you're right. After further review and refinement, there isn't significant functional commonality between the metrics and tracing handlers themselves. I thought inheritance or embedding thinking there could be shared setup or code reduction, but now I see the benefits of separation for clarity and maintainability are more important here.

purnesh42H · 2025-03-07T08:29:27Z

stats/opentelemetry/opentelemetry.go

-	return joinDialOptions(grpc.WithChainUnaryInterceptor(csh.unaryInterceptor), grpc.WithChainStreamInterceptor(csh.streamInterceptor), grpc.WithStatsHandler(csh))
+	var do []grpc.DialOption
+
+	if o.isMetricsEnabled() {


we don't need to check for MetricsEnabled as that's being added as no-op and check is there in initializeMetrics

purnesh42H · 2025-03-07T08:30:01Z

stats/opentelemetry/opentelemetry.go

-	return joinServerOptions(grpc.ChainUnaryInterceptor(ssh.unaryInterceptor), grpc.ChainStreamInterceptor(ssh.streamInterceptor), grpc.StatsHandler(ssh))
+	var so []grpc.ServerOption
+
+	if o.isMetricsEnabled() {


same. We don't need to check for metrics enabled

purnesh42H · 2025-03-07T08:34:27Z

stats/opentelemetry/opentelemetry.go

+			options: o,
+		}
+		tracingHandler.initializeTraces()
+		do = append(do,


same: one line

purnesh42H · 2025-03-07T08:34:28Z

stats/opentelemetry/opentelemetry.go

+		)
+	}
+	if o.isTracingEnabled() {
+		tracingHandler := &clientTracingHandler{


nit: one line

purnesh42H · 2025-03-07T08:34:46Z

stats/opentelemetry/opentelemetry.go

+	var do []grpc.DialOption
+
+	if o.isMetricsEnabled() {
+		metricsHandler := &clientStatsHandler{


why are we adding these new changes? We should keep the current part as is and only add the tracing part. Please revert the refactor changes done for clientStatsHandler here

csh := &clientStatsHandler{options: o} csh.initializeMetrics() do := joinDialOptions(grpc.WithChainUnaryInterceptor(csh.unaryInterceptor), grpc.WithChainStreamInterceptor(csh.streamInterceptor), grpc.WithStatsHandler(csh)) if o.isTracingEnabled(){ ... do := joinDialOptions(grpc.WithChainUnaryInterceptor(th.unaryInterceptor),.....) } return do

purnesh42H · 2025-03-07T09:11:26Z

stats/opentelemetry/metricsregistry_test.go

 }

 func newServerStatsHandler(options MetricsOptions) metricsRecorderForTest {
-	return &serverStatsHandler{options: Options{MetricsOptions: options}}
+	rm := &registryMetrics{optionalLabels: options.OptionalLabels}


same. We should't need to change anything here.

purnesh42H · 2025-03-07T09:11:27Z

stats/opentelemetry/metricsregistry_test.go

@@ -47,11 +47,13 @@ type metricsRecorderForTest interface {
 }

 func newClientStatsHandler(options MetricsOptions) metricsRecorderForTest {
-	return &clientStatsHandler{options: Options{MetricsOptions: options}}
+	rm := &registryMetrics{optionalLabels: options.OptionalLabels}


we shouldn't need to change anything here. Please revert.

purnesh42H · 2025-03-07T09:11:59Z

stats/opentelemetry/server_metrics.go

@@ -178,6 +201,14 @@ func (h *serverStatsHandler) TagConn(ctx context.Context, _ *stats.ConnTagInfo)
 // HandleConn exists to satisfy stats.Handler.
 func (h *serverStatsHandler) HandleConn(context.Context, stats.ConnStats) {}

+// TagConn exists to satisfy stats.Handler for tracing.
+func (h *serverTracingHandler) TagConn(ctx context.Context, _ *stats.ConnTagInfo) context.Context {
+	return ctx


nit: single line

After saving the file it's coming in a newline.

purnesh42H · 2025-03-07T09:11:59Z

stats/opentelemetry/server_metrics.go

@@ -170,6 +185,14 @@ func (h *serverStatsHandler) streamInterceptor(srv any, ss grpc.ServerStream, _
 	return err
 }

+func (h *serverTracingHandler) unaryInterceptor(ctx context.Context, req any, _ *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (any, error) {


nit: single line

After saving the file it's coming in a newline.

purnesh42H · 2025-03-07T09:12:38Z

stats/opentelemetry/client_metrics.go

+	}
+	ai := ri.ai
+
+	ctx, _ = h.traceTagRPC(ctx, ai)


nit: can directly pass ri.ai

purnesh42H · 2025-03-07T09:38:42Z

stats/opentelemetry/client_metrics.go

@@ -71,10 +76,18 @@ func (h *clientStatsHandler) initializeMetrics() {
 	rm.registerMetrics(metrics, meter)
 }

+func (h *clientTracingHandler) initializeTraces() {


same. Should go to client_tracing.go

purnesh42H · 2025-03-07T09:41:06Z

stats/opentelemetry/client_metrics.go

+}
+
+func (h *clientTracingHandler) unaryInterceptor(ctx context.Context, method string, req, reply any, cc *grpc.ClientConn, invoker grpc.UnaryInvoker, opts ...grpc.CallOption) error {
+	ci := &callInfo{


looks like we are setting callInfo again here. Since, tracingHandler is added after clientStatsHandler, the callInfo would have already been created. Similar to what we are doing with attempt info, we should try fetching here.

purnesh42H · 2025-03-07T09:41:35Z

stats/opentelemetry/client_metrics.go

+}
+
+func (h *clientTracingHandler) streamInterceptor(ctx context.Context, desc *grpc.StreamDesc, cc *grpc.ClientConn, method string, streamer grpc.Streamer, opts ...grpc.CallOption) (grpc.ClientStream, error) {
+	ci := &callInfo{


same comment about callInfo as unaryInterceptor of tracingHandler

purnesh42H · 2025-03-07T09:42:46Z

stats/opentelemetry/server_metrics.go

+	}
+	ai := ri.ai
+
+	ctx, _ = h.traceTagRPC(ctx, ai)


nit: can pass ri.ai directly

purnesh42H · 2025-03-07T09:43:42Z

stats/opentelemetry/server_metrics.go

+	}
+	ai := ri.ai
+
+	ctx, _ = h.traceTagRPC(ctx, ai)


we shouldn't ignore the attempt info because that contains the trace span. We should set it again in the context using setRPCInfo

vinothkumarr227 · 2025-03-10T10:02:57Z

stats/opentelemetry/client_tracing.go

+// Fetches rpcInfo from context. This handler requires a preceding stats handler
+// in the interceptor chain to have already created and set the rpcInfo.
+func (h *clientTracingHandler) TagRPC(ctx context.Context, _ *stats.RPCTagInfo) context.Context {
+	ri := getRPCInfo(ctx)


Add a comment explaining why we are fetching getRPCInfo

vinothkumarr227 · 2025-03-10T10:08:47Z

stats/opentelemetry/client_metrics.go

@@ -71,10 +68,12 @@ func (h *clientStatsHandler) initializeMetrics() {
 	rm.registerMetrics(metrics, meter)
 }

+// unaryInterceptor implements the UnaryClientInterceptor to record metrics for
+// unary calls.
 func (h *clientStatsHandler) unaryInterceptor(ctx context.Context, method string, req, reply any, cc *grpc.ClientConn, invoker grpc.UnaryInvoker, opts ...grpc.CallOption) error {


Add a doc string // unaryInterceptor records metrics for unary RPC calls. It updates the context
// with call info, adds plugin metadata if configured, and tracks call duration.

janardhankrishna-sai and others added 8 commits December 23, 2024 07:09

replace dial with newclient

7ddbe46

Merge branch 'master' of https://github.com/janardhanvissa/grpc-go

0486355

Merge branch 'grpc:master' into master

53fa9cd

Merge branch 'grpc:master' into master

4435b8a

Merge branch 'grpc:master' into master

a413555

Merge branch 'grpc:master' into master

4e203c3

Merge branch 'grpc:master' into master

e9ad552

refactor tracing and metrics interceptors separately

71804b4

janardhankrishna-sai added 2 commits February 3, 2025 11:21

adding opentelemetry tracing for client-server

4b3bd26

fixing vet issues

69df069

janardhanvissa force-pushed the refactor-tracing-metrics branch from 69df069 to 71804b4 Compare February 3, 2025 11:48

janardhankrishna-sai added 6 commits February 3, 2025 12:23

reverting newclient changes and fixing vet issues

770f430

reverting otel trace for client/server changes

b97a3ca

fixing vet issue

c89f3c9

renaming receiver name

3f07e48

unused param fix

5e8a4a5

fixing vet issue

76e422a

purnesh42H assigned janardhanvissa Feb 5, 2025

purnesh42H self-requested a review February 5, 2025 19:15

purnesh42H requested changes Feb 5, 2025

View reviewed changes

janardhankrishna-sai added 6 commits February 12, 2025 08:08

refactor client interceptor separately for traces and metrics

8fa0b03

moving tracing code to client_tracing.go file

1f41a49

revert previous commit

d74c61d

adding separate interceptors for traces and metrics of server

170eef6

separating HandleRPC interceptors of traces and metrics

7f5f539

updating client and server metrics

50999a0

purnesh42H assigned purnesh42H and unassigned janardhanvissa Feb 18, 2025

purnesh42H changed the title ~~opentelemetry: Refactor tracing and metrics separately~~ stats/opentelemetry: separate out interceptors for tracing and metrics Feb 19, 2025

purnesh42H reviewed Feb 19, 2025

View reviewed changes

purnesh42H assigned janardhanvissa and unassigned purnesh42H Feb 19, 2025

removing metrics code from tracingstatshandler and unused parameters

68b8966

purnesh42H reviewed Feb 21, 2025

View reviewed changes

janardhankrishna-sai and others added 11 commits February 24, 2025 11:11

addressing review comments

8b56f0f

addressing review comments

ac6080f

Merge branch 'master' into refactor-tracing-metrics

ed5506c

updating go.sum

be72377

fixing linter issue

a5ed115

addressing review comments for client

f243b43

addressing review comments for client

efb738f

Merge branch 'master' into refactor-tracing-metrics

7b97c65

addressing review comments for server

e8b0180

fixing vet issue

f70274b

updating newServerStatsHandler

5e607c5

janardhanvissa requested a review from purnesh42H March 7, 2025 04:43

janardhanvissa removed their assignment Mar 7, 2025

updating var name

a850532

purnesh42H reviewed Mar 7, 2025

View reviewed changes

janardhankrishna-sai added 2 commits March 10, 2025 05:58

moving tracing code to a separate file and adding comments

0f39f8d

removing unused isMetricsEnabled()

2a914e3

vinothkumarr227 reviewed Mar 10, 2025

View reviewed changes

stats/opentelemetry: separate out interceptors for tracing and metrics #8063

Are you sure you want to change the base?

stats/opentelemetry: separate out interceptors for tracing and metrics #8063

Conversation

janardhanvissa commented Feb 3, 2025 • edited by purnesh42H Loading

codecov bot commented Feb 3, 2025 • edited Loading

Codecov Report

purnesh42H commented Feb 5, 2025

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

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

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

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

janardhanvissa commented Feb 3, 2025 •

edited by purnesh42H

Loading

codecov bot commented Feb 3, 2025 •

edited

Loading