-
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.
|
|
||
| ### 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.
|
|
||
| ## Rationale | ||
|
|
||
| A single label is restrictive. We could allow applications to have multiple |
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.
Might also want to mention that supporting an arbitrary set of key/value pairs would likely impose some performance constraints that we'd prefer to avoid.
No description provided.