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/internal/xdsclient: Process string metadata in CDS for com.google.csm.telemetry_labels #7085

Merged
merged 3 commits into from
Apr 9, 2024

Conversation

zasweq
Copy link
Contributor

@zasweq zasweq commented Apr 3, 2024

This PR adds processing in CDS for string telemetry labels with filter_metadata string "com.google.csm.telemetry_labels". Other value types and filter_metadata identifiers are ignored. This also does not introduce NACK's. This is based off the C-Core implementation, which I disagree with technically but understand there were good reasons for the decisions made. See discussion in xDS Chat room for context as to why C-Core did it this way and why I followed it.

The relevant language in the CSM Observability Design is "The above key_value pairs from com.google.csm.telemetry_labels are going to be parsed by xDS Client from the CDS Resource and propagated to the watcher on this resource in the “cds” load balancing policy."

RELEASE NOTES: N/A

@zasweq zasweq requested a review from dfawley April 3, 2024 21:01
@zasweq zasweq modified the milestones: 1.63 Release, 1.64 Release Apr 3, 2024
@zasweq zasweq added the Type: Feature New features or improvements in behavior label Apr 3, 2024
Copy link

codecov bot commented Apr 3, 2024

Codecov Report

Merging #7085 (dcc290c) into master (4ec8307) will decrease coverage by 1.36%.
Report is 7 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7085      +/-   ##
==========================================
- Coverage   82.54%   81.19%   -1.36%     
==========================================
  Files         305      345      +40     
  Lines       31393    33940    +2547     
==========================================
+ Hits        25913    27557    +1644     
- Misses       4425     5212     +787     
- Partials     1055     1171     +116     
Files Coverage Δ
...ds/internal/xdsclient/xdsresource/unmarshal_cds.go 86.19% <100.00%> (+0.41%) ⬆️

... and 65 files with indirect coverage changes

xds/internal/xdsclient/xdsresource/type_cds.go Outdated Show resolved Hide resolved
xds/internal/xdsclient/xdsresource/unmarshal_cds.go Outdated Show resolved Hide resolved
xds/internal/xdsclient/xdsresource/unmarshal_cds.go Outdated Show resolved Hide resolved
if fields := val.GetFields(); fields != nil {
if val, ok := fields["service_name"]; ok {
if _, isStringVal := val.GetKind().(*structpb.Value_StringValue); isStringVal {
stringMD["service_name"] = val.GetStringValue()
Copy link
Member

Choose a reason for hiding this comment

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

This feels weird. We check the type of a thing, then call GetStringValue on some other thing. Does GetStringValue do something different if the Kind() is not StringValue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is the best solution that I could come up with. GetStringValue() returns a string, not a *string, so there is no way to distinguish between an empty string (a valid label value, which will be recorded) and an unset string (should eventually record method labels as "unknown" if unset). Thus, I don't set if not a string value (not set = "unknown" in OpenTelemetry component) and set it even if it's empty if it's present.

Copy link
Member

Choose a reason for hiding this comment

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

I believe you can do:

if s, ok := val.AsInterface().(string); ok {
	stringMD["service_name"] = s
}

But I'm not sure that that's much better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no function on the val called AsInterface() :/. Also, I think that typecast is wrong because it's a wrapper interface which happens to contain the value string. I.e. there's another layer (not 100% sure on this, but pretty sure).

@dfawley dfawley assigned zasweq and unassigned dfawley Apr 9, 2024
@zasweq zasweq merged commit 308dbc4 into grpc:master Apr 9, 2024
14 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 7, 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