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

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

RELEASE NOTES: None

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

Codecov Report

Attention: Patch coverage is 69.88636% with 53 lines in your changes missing coverage. Please review.

Project coverage is 81.93%. Comparing base (775150f) to head (5bb48b3).
Report is 11 commits behind head on master.

Files with missing lines	Patch %	Lines
stats/opentelemetry/client_tracing.go	58.57%	23 Missing and 6 partials ⚠️
stats/opentelemetry/server_tracing.go	66.66%	7 Missing and 4 partials ⚠️
stats/opentelemetry/client_metrics.go	79.54%	6 Missing and 3 partials ⚠️
stats/opentelemetry/server_metrics.go	73.33%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8063      +/-   ##
==========================================
- Coverage   82.25%   81.93%   -0.33%     
==========================================
  Files         393      410      +17     
  Lines       39143    40300    +1157     
==========================================
+ Hits        32197    33019     +822     
- Misses       5616     5901     +285     
- Partials     1330     1380      +50

Files with missing lines	Coverage Δ
stats/opentelemetry/opentelemetry.go	`76.54% <100.00%> (+1.54%)`	⬆️
stats/opentelemetry/server_metrics.go	`87.73% <73.33%> (-2.10%)`	⬇️
stats/opentelemetry/client_metrics.go	`83.33% <79.54%> (-6.07%)`	⬇️
stats/opentelemetry/server_tracing.go	`74.41% <66.66%> (-25.59%)`	⬇️
stats/opentelemetry/client_tracing.go	`64.19% <58.57%> (-12.28%)`	⬇️

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

stats/opentelemetry/client_metrics.go

stats/opentelemetry/client_tracing.go

stats/opentelemetry/server_metrics.go

stats/opentelemetry/server_tracing.go

purnesh42H

Please consider the same comments for server metrics and tracing as well.

stats/opentelemetry/client_metrics.go

stats/opentelemetry/client_tracing.go

stats/opentelemetry/opentelemetry.go

stats/opentelemetry/client_tracing.go

stats/opentelemetry/client_metrics.go

stats/opentelemetry/client_tracing.go

stats/opentelemetry/client_metrics.go

stats/opentelemetry/server_tracing.go

stats/opentelemetry/server_metrics.go

stats/opentelemetry/client_metrics.go

stats/opentelemetry/server_metrics.go

purnesh42H · 2025-03-25T18:19:35Z

stats/opentelemetry/server_metrics.go

 	method := info.FullMethodName
 	if h.options.MetricsOptions.MethodAttributeFilter != nil {
 		if !h.options.MetricsOptions.MethodAttributeFilter(method) {
-			method = "other"


why are you changing this? This is suppose to be other if the method is part of the filtered list. Please revert and don't make unnecessary changes

purnesh42H · 2025-03-25T18:19:51Z

stats/opentelemetry/server_metrics.go

 		}
 	}
 	server := internal.ServerFromContext.(func(context.Context) *grpc.Server)(ctx)
 	if server == nil { // Shouldn't happen, defensive programming.
 		logger.Error("ctx passed into server side stats handler has no grpc server ref")
-		method = "other"
+		method = methodName


same. Please revert

purnesh42H · 2025-03-25T18:20:02Z

stats/opentelemetry/server_metrics.go

 	} else {
 		isRegisteredMethod := internal.IsRegisteredMethod.(func(*grpc.Server, string) bool)
 		if !isRegisteredMethod(server, method) {
-			method = "other"
+			method = methodName


purnesh42H · 2025-03-25T18:23:14Z

stats/opentelemetry/server_metrics.go

 func (h *serverStatsHandler) TagRPC(ctx context.Context, info *stats.RPCTagInfo) context.Context {
+	ri := getRPCInfo(ctx)


line 185-191 should be down where you are removing attemptInfo stuff. Don't move it at the start. As i mentioned in the office hours, we follow convention of declaring the identifier right before its needed.

purnesh42H · 2025-03-25T18:24:24Z

stats/opentelemetry/server_metrics.go

 }

-// HandleRPC implements per RPC tracing and stats implementation.
+// HandleRPC handles per RPC attempt stats for server-side metrics collection.


Suggested change

// HandleRPC handles per RPC attempt stats for server-side metrics collection.

// HandleRPC handles per RPC stats implementation.

purnesh42H · 2025-03-25T18:25:52Z

stats/opentelemetry/client_metrics.go

 func (h *clientStatsHandler) TagRPC(ctx context.Context, info *stats.RPCTagInfo) context.Context {
+	ri := getRPCInfo(ctx)
+	var ai *attemptInfo


same. Please move this entire thing down where you are removing attemptInfo stuff.

purnesh42H · 2025-03-25T18:28:02Z

stats/opentelemetry/client_tracing.go

+
+// HandleRPC handles per-RPC attempt stats events for tracing.
+func (h *clientTracingHandler) HandleRPC(ctx context.Context, rs stats.RPCStats) {
+	// Fetch the rpcInfo set by a previously registered stats handler.


no need of this comment

purnesh42H · 2025-03-25T18:28:45Z

stats/opentelemetry/server_metrics.go

 )

+const methodName = "other"


Again. Why this unnecessary change? Please revert

purnesh42H · 2025-03-25T18:31:02Z

stats/opentelemetry/server_tracing.go

@@ -44,3 +80,22 @@ func (h *serverStatsHandler) traceTagRPC(ctx context.Context, ai *attemptInfo) (
 	ai.traceSpan = span
 	return ctx, ai
 }
+
+// HandleRPC handles per-RPC attempt stats events for tracing.


Suggested change

// HandleRPC handles per-RPC attempt stats events for tracing.

// HandleRPC handles per RPC tracing implementation.

purnesh42H · 2025-03-25T18:31:13Z

stats/opentelemetry/server_tracing.go

+
+// HandleRPC handles per-RPC attempt stats events for tracing.
+func (h *serverTracingHandler) HandleRPC(ctx context.Context, rs stats.RPCStats) {
+	// Fetch the rpcInfo set by a previously registered stats handler.


no need of this comment.

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

janardhankrishna-sai and others added 7 commits March 13, 2025 10:59

updating logger

e5f50b7

Merge branch 'master' into refactor-tracing-metrics

ffcc0fa

update unaryInterceptor

67e5b24

revert doc string

8cf7f15

fixing vet issue

7949501

update clientStatsHandler unaryInterceptor

ad22a22

updating verbosity level and removing redundant code

dd179fb

purnesh42H reviewed Mar 13, 2025

View reviewed changes

janardhankrishna-sai and others added 2 commits March 13, 2025 18:58

reverting logger.Info from logger.Error

3201805

Update server_metrics.go

a3915ef

janardhanvissa requested a review from purnesh42H March 13, 2025 19:17

purnesh42H added Type: Internal Cleanup Refactors, etc Area: Observability Includes Stats, Tracing, Channelz, Healthz, Binlog, Reflection, Admin, GCP Observability labels Mar 13, 2025

purnesh42H added this to the 1.72 Release milestone Mar 13, 2025

purnesh42H reviewed Mar 18, 2025

View reviewed changes

addressing review comments

1ff5159

janardhanvissa requested a review from purnesh42H March 18, 2025 14:01

updating the comment

4133dd3

purnesh42H reviewed Mar 20, 2025

View reviewed changes

purnesh42H assigned janardhanvissa Mar 20, 2025

janardhankrishna-sai added 3 commits March 20, 2025 08:50

addressing review comments

f8e0cac

fixing vet issue

0b89cf6

fixing linter issue

42c08a0

janardhanvissa requested a review from purnesh42H March 20, 2025 09:11

janardhanvissa removed their assignment Mar 20, 2025

purnesh42H reviewed Mar 24, 2025

View reviewed changes

stats/opentelemetry/client_metrics.go Show resolved Hide resolved

stats/opentelemetry/server_metrics.go Show resolved Hide resolved

purnesh42H assigned janardhanvissa Mar 24, 2025

addressing review comments

5bb48b3

janardhanvissa requested a review from purnesh42H March 25, 2025 14:03

purnesh42H reviewed Mar 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

janardhanvissa commented Feb 3, 2025 •

edited by purnesh42H

Loading

codecov bot commented Feb 3, 2025 •

edited

Loading

purnesh42H commented Feb 5, 2025

purnesh42H left a comment

purnesh42H left a comment

purnesh42H Mar 25, 2025

purnesh42H Mar 25, 2025

purnesh42H Mar 25, 2025

purnesh42H Mar 25, 2025

purnesh42H Mar 25, 2025

purnesh42H Mar 25, 2025

purnesh42H Mar 25, 2025

purnesh42H Mar 25, 2025

purnesh42H Mar 25, 2025

purnesh42H Mar 25, 2025

		func (h serverStatsHandler) TagRPC(ctx context.Context, info stats.RPCTagInfo) context.Context {
		ri := getRPCInfo(ctx)

	// HandleRPC handles per RPC attempt stats for server-side metrics collection.
	// HandleRPC handles per RPC stats implementation.

	// HandleRPC handles per-RPC attempt stats events for tracing.
	// HandleRPC handles per RPC tracing implementation.

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

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

janardhanvissa commented Feb 3, 2025 •

edited by purnesh42H

Loading

codecov bot commented Feb 3, 2025 •

edited

Loading