-
Notifications
You must be signed in to change notification settings - Fork 4.6k
internal/stats: Moved Labels to experimental/stats package
#8671
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
internal/stats: Moved Labels to experimental/stats package
#8671
Conversation
Signed-off-by: Seth Epps <[email protected]>
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8671 +/- ##
==========================================
+ Coverage 81.76% 83.02% +1.25%
==========================================
Files 417 417
Lines 40806 32296 -8510
==========================================
- Hits 33367 26815 -6552
+ Misses 6057 4097 -1960
- Partials 1382 1384 +2
🚀 New features to boost your workflow:
|
eshitachandwani
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.
LGTM. Deferring to @easwars for another review.
|
I believe the decision to make these internal was made deliberately for cross-language parity reasons. So we might not want to do this without checking with the other languages first. |
Since we need a cross language discussion before making this change.
|
@eshitachandwani Let me know if there's anything I can help with / dig into regarding the other implementations Thinking about this particular move, I could maybe try to tweak things a bit and offer an something like // package experimental/stats
import (
"context"
istats "google.golang.org/grpc/internal/stats"
)
// InitStatLabels configures the context stat labels. WARNING: this should not be used when configuring metric collection with an OpenTelemetry provider
func InitStatLabels(ctx context.Context) {
var labels *istats.Labels
if labels = istats.GetLabels(ctx); labels == nil {
labels = &estats.Labels{
TelemetryLabels: map[string]string{},
}
ctx = istats.SetLabels(ctx, labels)
}
// GetLabels returns the Labels stored in the context, or nil if there is one.
func GetLabels(ctx context.Context) *Labels {
// !!! labelsKey isn't exported by istats
labels, _ := ctx.Value(labelsKey{}).(*Labels)
return labels
}But that does introduce and import cycle and there would need to be some extra refactoring to expose the context key, which I'm not sure we would want to go down that rabbit hole |
|
Hey @seth-epps , as Doug pointed out , we might want to look at other language implementation if they are have the functionality to expose these functions and if yes how do they do this and we might want to do something similar. In the mean time , we could open an issue and discuss your use case and figure out if there could be some workaround for your particular use case. |
|
Closing the PR to discuss the issue and possible solutions on the issue : #8682 |
Related to #8446
This follows up on the discussion here
By moving
Labelsand the associated Get/Set functions to the exportedexperimental/statspackage we give client/server implementations ofstats.Handleraccess to telemetry labels, otherwise only accessible if instrumented using an OpenTelemetry provider.With this change, a handler implementation can start tracking telemetry labels by calling
SetLabelsinTagRPC/Tag ConnRELEASE NOTES:
Labelsto experimental/stats package