-
Notifications
You must be signed in to change notification settings - Fork 249
A108: OpenTelemetry Custom Per-Call Metric Label #525
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
|
|
||
| ### Go | ||
|
|
||
| gRPC Go will add a new API to set and get the custom label value in |
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.
|
|
||
| ### C++ | ||
|
|
||
| gRPC C++ will add an API to ClientContext. Precise details TBD. |
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.
@markdroth, can you help me flesh this out? It looks like y'all will need specialized APIs/plumbing.
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'll chat with @ctiller after the holidays to flesh this out, and I'll get back to you.
|
CC @ctiller |
markdroth
left a comment
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.
Overall, this looks good! I just want to chat with @ctiller before approving, just to make sure we've dotted our I's and crossed our T's.
| * `grpc.client.call.retry_delay` | ||
| * Other per-call instruments, e.g., those added by an LB policy, are encouraged | ||
| to support this label | ||
| * RLS is the only such case today, but is not defined in a gRFC |
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'd suggest just removing this bullet, since it isn't actually a public API. If we want this label on these metrics, we can figure that out internally.
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.
Unless there is a strong objection, I'm going to leave it, because "figure it out internally" has not been going well, and I basically need to define nothing here. And the code is OSS and I made all the changes to Java in the same commit.
Without this bullet, I'd also need to say "there are no such cases today" to be clear I'm giving the exhaustive list, but that comment would be incorrect because RLS does exist.
|
|
||
| ### C++ | ||
|
|
||
| gRPC C++ will add an API to ClientContext. Precise details TBD. |
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'll chat with @ctiller after the holidays to flesh this out, and I'll get back to you.
No description provided.