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: Emit unknown for CSM Labels if not present in CDS #7309

Merged
merged 3 commits into from
Jun 10, 2024

Conversation

zasweq
Copy link
Contributor

@zasweq zasweq commented Jun 6, 2024

This PR fixes CSM interop tests. Specifically, it aligns with the language from CSM Design: "The values for the service labels csm.service_name and csm.service_namespace_name come from xDS, “unknown” if not present."

This behavior lives in the xDS Client due to CSM Label filtering already happening in the xDS Client, and to keep in one place. Previously we had filtering in plugin option as well, but this behavior keeps it in one place. The only way these optional labels to be used is to configure an OpenTelemetry Dial Option with both of these optional labels set in API.

RELEASE NOTES: N/A

@zasweq zasweq requested a review from easwars June 6, 2024 23:59
@zasweq zasweq added this to the 1.65 Release milestone Jun 6, 2024
Comment on lines 88 to 91
TelemetryLabels: map[string]string{
"csm.service_name": "unknown",
"csm.service_namespace_name": "unknown",
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make a package global and reference it from individual tests.

unknownCSMLabels = map[string]string{
	"csm.service_name":           "unknown",
	"csm.service_namespace_name": "unknown",
},

I understand this could be a little counter to other comments that we make where we say test logic and state should stay as local as possible, instead of being spread in different places. But these labels, I feel (correct me if I'm wrong), are not very pertinent to the test, and someone reading the test need not know what these labels mean to understand what the test is doing. So, I feel we can make the test tables a tiny bit smaller without losing the readability of them.

This comment applies to other tests as well.

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, check out the line diff of commit :).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first I created symbol in xds/internal/testutils, but that had an import cycle since that imports xdsresource and xdsresource accesses symbol, so I just put in xds/internal.

@sergiitk
Copy link
Member

sergiitk commented Jun 7, 2024

For posterity: the issue was caught in grpc/psm-interop#98.

Ad-hoc run to verify the fix:

@easwars
Copy link
Contributor

easwars commented Jun 7, 2024

@zasweq : I reviewed this yesterday, but forgot to assign it back to you.

@zasweq zasweq assigned easwars and unassigned zasweq Jun 7, 2024
@easwars easwars assigned zasweq and unassigned easwars Jun 10, 2024
@zasweq zasweq merged commit e2e7a51 into grpc:master Jun 10, 2024
11 checks passed
@zasweq
Copy link
Contributor Author

zasweq commented Jun 10, 2024

Will need to cherry pick this onto 1.65.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants