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

xds: Process telemetry labels from CDS in xDS Balancers #7116

Merged
merged 3 commits into from
Apr 15, 2024

Conversation

zasweq
Copy link
Contributor

@zasweq zasweq commented Apr 9, 2024

This PR plumbs the Telemetry Labels from CDS through our xDS Balancer tree through the context that eventually get passed to stats handler.

RELEASE NOTES: N/A

@zasweq zasweq requested a review from dfawley April 9, 2024 22:06
@zasweq zasweq added the Type: Feature New features or improvements in behavior label Apr 9, 2024
@zasweq zasweq added this to the 1.64 Release milestone Apr 9, 2024
@zasweq zasweq force-pushed the xds-telemetry-labels branch from f0b77b1 to 6f853db Compare April 9, 2024 22:13
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 a few nits


type labelsKey struct{}

// GetLabels returns the Labels stored in theo context, or nil if there is one
Copy link
Member

Choose a reason for hiding this comment

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

theo* and end sentence with punctuation pls.

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.

return labels
}

// SetLabels sets the Labels
Copy link
Member

Choose a reason for hiding this comment

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

Please finish the sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in the context.

Comment on lines 31 to 32
testgrpc "google.golang.org/grpc/interop/grpc_testing"
testpb "google.golang.org/grpc/interop/grpc_testing"
Copy link
Member

Choose a reason for hiding this comment

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

Please group pbs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Argh, this slipped through. Switched.

}

func newPicker(s balancer.State, config *dropConfigs, loadStore load.PerClusterReporter) *picker {
func newPicker(s balancer.State, config *dropConfigs, loadStore load.PerClusterReporter, telemetryLabels map[string]string) *picker {
Copy link
Member

Choose a reason for hiding this comment

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

Shall we make this a method on the balancer instead of adding more parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, done.

for key, value := range d.telemetryLabels {
labels.TelemetryLabels[key] = value
}
} // Unconditionally set, even dropped or queued RPC's can use this label.
Copy link
Member

Choose a reason for hiding this comment

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

Move to above if info.Ctx != nil or just inside the open brace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added above the info.Ctx != nil check, the comment turned out to be: Unconditionally set labels if present, even dropped or queued RPC's can use these labels.

// handler asserts that subsequent HandleRPC calls from the RPC lifecycle
// contain telemetry labels that it can see.
func (s) TestTelemetryLabels(t *testing.T) {
managementServer, nodeID, _, resolver, cleanup1 := e2e.SetupManagementServer(t, e2e.ManagementServerOptions{})
Copy link
Member

Choose a reason for hiding this comment

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

s/cleanup1/cleanup/ please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, switched.

Comment on lines +125 to +126
// aren't started. All of these should have access to the desired telemetry
// labels.
Copy link
Member

Choose a reason for hiding this comment

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

If you mean to test all 3, you will need a continue on the cases. Or case a, b, c:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops switched.

@dfawley dfawley assigned zasweq and unassigned dfawley Apr 15, 2024
@zasweq zasweq merged commit b37cd81 into grpc:master Apr 15, 2024
12 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants