client: process RPC stats/tracing only when a handler is configured

[mbissa]

This resolves an issue where below log statement keeps printing repeatedly:
"ERROR: [otel-plugin] ctx passed into client side stats handler metrics event handling has no client attempt data present".
When new stream is created for health/orca producers, stats and tracing is not setup. However, this fact is ignored during RPC and an error logs is printed to denote that stats cannot be handled. We will enable stream to have its own reference to the stats handler and only process per RPC implementation when it is present (like in case of regular data streams).

Internal issue: b/385685802

RELEASE NOTES:

• stats: only process RPC stats/tracing in health and ORCA producers if a handler is configured, preventing unnecessary error logging

[codecov]

Codecov Report

❌ Patch coverage is 60.00000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.70%. Comparing base (2c113ab) to head (942f259).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
internal/transport/http2_client.go	53.84%	0 Missing and 6 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8874      +/-   ##
==========================================
- Coverage   81.41%   80.70%   -0.72%     
==========================================
  Files         416      416              
  Lines       33429    33434       +5     
==========================================
- Hits        27216    26982     -234     
+ Misses       4660     4648      -12     
- Partials     1553     1804     +251     

Files with missing lines	Coverage Δ
internal/transport/client_stream.go	82.22% <ø> (-17.78%)	⬇️
internal/transport/transport.go	89.06% <ø> (-2.09%)	⬇️
stream.go	76.38% <100.00%> (-5.50%)	⬇️
internal/transport/http2_client.go	72.39% <53.84%> (-19.96%)	⬇️

... and 23 files with indirect coverage changes

🚀 New features to boost your workflow:

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

[easwars]

Curious, how did you run into this? Is there an issue for this?

[easwars]

When new stream is created for health/orca producers, stats and tracing is not setup

Can you please elaborate on this?

My understanding is that when the opentelemtry.DialOption is specified at channel creation time, the gRPC channel is configured with the oTel stats handler. And this is stored in the cc.statsHandler field which is what is what gets propagated to the RPC attempt struct csAttempt in newAttemptLocked(). So, this information should also be available when creating the health or ORCA stream, right? Or is it the case that statstracing should be disabled on these streams?

I think it would be good to open an issue that documents all the details and the approach taken (and why this is the preferred approach, if there are other approached available at all). Thanks.

[easwars]

Is this how the health and ORCA streams get a nil stats handler? We want to eliminate this duplication between the regular client stream and the non-retry client stream at some point. We tried to prioritize that work for Q1, but it didn't happen. This has been a maintenance burden for a while now, and a source of hard to find bugs.

[mbissa]

Just did a rebase with master to fast forward my commit and address the vet-proto failure.

[arjan-bal]

Just FYI, no action needed. I was considering whether we'd need to pass MetricsRecorderList separately to skip metrics recording. After reviewing the code for references, I found that MetricsRecorderList is only stored in the ClientConn struct and used for subchannel metrics. Stream metrics, on the other hand, are recorded via calls to the stats handlers.

grpc-go/stats/opentelemetry/client_metrics.go

Lines 192 to 199 in d7b3f93

	func (h *clientMetricsHandler) HandleRPC(ctx context.Context, rs stats.RPCStats) {
	ri := getRPCInfo(ctx)
	if ri == nil {
	logger.Error("ctx passed into client side stats handler metrics event handling has no client attempt data present")
	return
	}
	h.processRPCEvent(ctx, rs, ri.ai)
	}

Consequently, this change should also eliminate the error log from the OTel metrics handler as well. As before, per-call metrics will not be recorded for RPCs initiated by producers.

[easwars]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors how RPC stats and tracing are handled to resolve an issue with repetitive error logging for health and ORCA producers. The change introduces a per-stream stats.Handler, moving away from a transport-level handler for RPC events. This allows streams like those for health checks, which don't have stats configured, to bypass stats processing, thus preventing the erroneous log messages. The implementation is clean and effective: the stats.Handler is now part of the ClientStream struct and is passed during stream creation. The NewStream function signature has been updated across the transport layer, and call sites have been correctly modified. For regular RPCs, the existing stats handler is plumbed through, while for internal streams like health checks, nil is passed, correctly disabling stats for them. The changes are logical and well-executed. This is a good improvement to reduce log noise and make stats handling more precise.

[easwars]

Would it be possible to add a test that verifies that the offending log is not seen for health/ORCA streams?

[easwars]

Can we please add a short trailing comment explaining when this can be nil? Thanks.

[mbissa]

done.

[easwars]

Can we please add a comment here as to why we are setting it to nil here, so that future readers dont have to second guess themselves.

[mbissa]

done

[easwars]

I see that a stats.Handler field is stored in the transport now. Is it required anymore at all given that the stats handler is passed at stream creation time?

[mbissa]

We can look at all the details and address it as part of bigger cleanup - don't want to include more changes here as this is an urgent change.