Skip to content
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

stats: Add optional locality label in cluster_impl picker #7434

Merged
merged 5 commits into from
Jul 25, 2024

Conversation

zasweq
Copy link
Contributor

@zasweq zasweq commented Jul 23, 2024

This PR adds the setting of the optional locality label in the cluster_impl picker for per call metrics. It also sets the default of "" for this label in OpenTelemetry.

RELEASE NOTES: N/A

@zasweq zasweq requested a review from dfawley July 23, 2024 00:23
@zasweq zasweq added the Type: Feature New features or improvements in behavior label Jul 23, 2024
@zasweq zasweq added this to the 1.66 Release milestone Jul 23, 2024
Copy link

codecov bot commented Jul 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.51%. Comparing base (0231b0d) to head (c546565).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7434      +/-   ##
==========================================
+ Coverage   81.44%   81.51%   +0.06%     
==========================================
  Files         352      352              
  Lines       26919    26927       +8     
==========================================
+ Hits        21924    21949      +25     
+ Misses       3804     3795       -9     
+ Partials     1191     1183       -8     
Files Coverage Δ
xds/internal/balancer/clusterimpl/picker.go 97.46% <100.00%> (+0.28%) ⬆️

... and 18 files with indirect coverage changes

Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM just nits.

stats/opentelemetry/client_metrics.go Outdated Show resolved Hide resolved
stats/opentelemetry/client_metrics.go Outdated Show resolved Hide resolved
test/xds/xds_telemetry_labels_test.go Outdated Show resolved Hide resolved
test/xds/xds_telemetry_labels_test.go Outdated Show resolved Hide resolved
xds/internal/balancer/clusterimpl/picker.go Outdated Show resolved Hide resolved
@dfawley dfawley assigned zasweq and unassigned dfawley Jul 23, 2024
@zasweq zasweq assigned dfawley and unassigned zasweq Jul 24, 2024
@zasweq
Copy link
Contributor Author

zasweq commented Jul 24, 2024

Got to all comments; no approval yet though so assigning back to you.

@@ -150,7 +150,12 @@ func (h *clientStatsHandler) TagRPC(ctx context.Context, info *stats.RPCTagInfo)
var labels *istats.Labels
if labels = istats.GetLabels(ctx); labels == nil {
labels = &istats.Labels{
TelemetryLabels: make(map[string]string),
// The defaults for all the per call label from a plugin that
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nits:

"labelS"

"on the call path"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done for both.

@dfawley dfawley assigned zasweq and unassigned dfawley Jul 24, 2024
@zasweq zasweq merged commit 1feeaec into grpc:master Jul 25, 2024
13 checks passed
printchard pushed a commit to printchard/grpc-go that referenced this pull request Jul 30, 2024
printchard pushed a commit to printchard/grpc-go that referenced this pull request Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants