-
Notifications
You must be signed in to change notification settings - Fork 4k
Add custom label for per-RPC metrics #12552
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
TODO: Add tests
|
@kannanjgithub, I added you as you may want to pre-review, before the gRFC is finalized. I'm really only expecting bikeshedding on the label name (as part of the gRFC). This is introducing a new stable API, so take a special look at the Grpc class. |
|
Thanks for putting this up. Changes look good. |
The existing tests really liked to use inconsistent CallOptions in the same test.
| if (module.customLabelEnabled) { | ||
| builder.put( | ||
| CUSTOM_LABEL_KEY, info.getCallOptions().getOption(Grpc.CALL_OPTION_CUSTOM_LABEL)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that javadoc for CALL_OPTION_CUSTOM_LABEL mentions "must not be set to null", however if an application developer inadvertently passes null using withOption, and here it will pass that null directly to the OpenTelemetry AttributesBuilder.put() method. Should we do an explicit null check here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or use of Strings.nullToEmpty().
TODO: Add tests
I've created this so I can reference it as an example from gRFC A108.