grpc/stats: Add support for custom labels in per call metrics (A108)#9008
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9008 +/- ##
==========================================
- Coverage 83.05% 82.96% -0.10%
==========================================
Files 411 411
Lines 32918 32960 +42
==========================================
+ Hits 27340 27345 +5
- Misses 4184 4209 +25
- Partials 1394 1406 +12
🚀 New features to boost your workflow:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new API, grpc.NewContextWithCustomLabel, allowing users to attach a custom string label to a gRPC call's context. This custom label is then propagated and recorded as an optional attribute in OpenTelemetry client-side metrics and RLS load balancer metrics, enabling more granular metric analysis. Feedback includes addressing an inconsistency in optional label handling within client_metrics.go to ensure consistent metric attributes, and an optimization to fetch the custom label once in balancer/rls/picker.go.
easwars
left a comment
There was a problem hiding this comment.
Which of the existing tests verifies that the label is attached to all existing per-call metrics?
There is no common test for verifying all labels for all metrics, instead there are tests for metrics that verify labels as well, I have added the check for custom labels there. |
| } | ||
| for _, o := range h.options.MetricsOptions.OptionalLabels { | ||
| if o == "grpc.client.call.custom" { | ||
| label := estats.CustomLabelFromContext(ctx) |
There was a problem hiding this comment.
Why do we check for label != "" down below but not here?
There was a problem hiding this comment.
This is correct, we should not be checking there as well. The fact that o == "grpc.client.call.custom" is true means the user has explicitly asked for this metric label to be present and even if the value is empty we should pass it through (considering the user has a use case when they want this value to be empty).
Also, in general in otel plugin also, if we have a optional label defined, we need to pass a value - even if empty string else it fails.
I have corrected it down below. Thanks for catching this!
Related Issues/gRFC : A108.
It introduces a mechanism for application authors to inject a custom, request-scoped string label (grpc.client.call.custom) into their RPCs via context.Context, which is then extracted and propagated directly to client-side per-call metrics.
Added
grpc.NewContextWithCustomLabel(ctx, label)andgrpc.CustomLabelFromContext(ctx)to provide a type-safe, canonical way for clients to attach the label to metrics associated with their outgoing RPCs.RELEASE NOTES:
grpc.NewContextWithCustomLabel(ctx, label)andgrpc.CustomLabelFromContext(ctx)) to allow clients to attachgrpc.client.call.customlabel with specified value to per-call metrics as part of implementing A108.