experimental/stats: Expose Telemetry Label Callback#8877
Conversation
seth-epps
force-pushed
the
i-8682/expose-recorder
branch
from
09e6612 to
6dda7c7
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8877 +/- ##
==========================================
+ Coverage 80.40% 83.11% +2.71%
==========================================
Files 416 414 -2
Lines 33495 33484 -11
==========================================
+ Hits 26930 27830 +900
+ Misses 4682 4230 -452
+ Partials 1883 1424 -459
🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| labels["grpc.lb.locality"] = xdsinternal.LocalityString(lID) | ||
| labels["grpc.lb.backend_service"] = d.clusterName | ||
| } | ||
| stats.UpdateLabels( |
There was a problem hiding this comment.
Can we restrict UpdateLabels to only one invocation?
There was a problem hiding this comment.
I did this in two separate invocations because I wanted to ensure the callbacks were executed in the event of an error above (dropped). If I combine the two we'd potentially miss the label updates
|
Hey @seth-epps , apologies for the delay and thanks for your effort. After discussing with other maintainers, I have added a few more comments. Please have a look and let me know if you have any questions around them. |
|
That test failure looks like a transient / unrelated flake |
|
@easwars Any chance you've had some time to re-review? 🙏 |
| // To ensure callbacks do not mutate the state of the provided label map it is copied | ||
| // before execution. |
There was a problem hiding this comment.
Can we drop this copy and instead document on LabelCallback that callbacks should not mutate the provided label map?
There was a problem hiding this comment.
This was done based on the gemini review, which maybe I put too much stock into.
#8877 (comment)
I think my only reservation with dropping the copy is that not enforcing the copy means that someone can interfere with the otel client metrics if they register something in addition and don't pay attention to the recommendation that callbacks not modify the map. That said, I don't know how much guardrails need to be put in place to avoid people making those types of mistakes. I don't mind removing it, let me know if you think the tradeoff is reasonable
Apologies for the delay. I was finally able to make a full pass. Mostly minor comments this time around. We should be able to wrap this one up quickly. Thanks. |
|
This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed. |
|
@easwars I've addressed most of the feedback, but left a couple notes for the larger discussion points, namely around the maps copy / replacement in the otel callbacks. Let me know what you think! |
|
@easwars Just wanted to check in on the latest changes to see if there was any additional feedback / if anything needs to be tweaked |
easwars
left a comment
easwars
left a comment
There was a problem hiding this comment.
LGTM
One super nit in this round and you now have a merge conflict to resolve.
Apologies for the long turnaround time on this review.
| // Package telemetry defines stats APIs for interacting with | ||
| // telemetry labels. | ||
| // | ||
| // All APIs in this package are experimental. |
There was a problem hiding this comment.
Nit: Could you please make this notice match some of the other notices that we have:
// # Experimental
//
// Notice: All APIs in this package are EXPERIMENTAL and may be changed or
// removed in a later release.
7 participants
Fixes #8682
Expose a new experimental API for registering a telemetry label callback function.
Some clients may not be instrumented with opentelemetry which restricts valuable information from being propagated to stats handlers. This gives clients the ability to collect otel labels by registering a label callback on the context and collecting the information themselves in their stats handlers.
RELEASE NOTES: