-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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/opentelemetry: Add CSM Plugin Option #7205
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7205 +/- ##
==========================================
- Coverage 81.24% 80.53% -0.72%
==========================================
Files 345 349 +4
Lines 33941 34063 +122
==========================================
- Hits 27574 27431 -143
- Misses 5202 5437 +235
- Partials 1165 1195 +30 |
df959cb
to
b142976
Compare
// Append the optional labels. To avoid string comparisons, assume the | ||
// caller only passes in two potential xDS Optional Labels: service_name and | ||
// service_namespace. | ||
for k, v := range optionalLabels { | ||
labels[k] = v | ||
} |
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.
Why don't we merge these labels externally, rather than passing them in here and merging them?
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.
Yeah, this confused me too. This is a built in function of how this API should work (one API call that emits labels from the three sources described above). Thus, it is intended for this layer to receive this arbitrary map of optional labels and make the determination as to which it is interested in. For this case, we assume the caller has processed these service_name and service_namespace for the sake of avoiding string comparisons.
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.
So why not remove it from the API then, if we're just going to use whatever is passed in?
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.
There can be optional labels other than service_name and service_namespace. Immediately, we have "locality". Additionally, the CSM Plugin Options changes the name of the label from "service_name" to "csm.service_name".
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.
Oh whoops I didn't prepend csm. for the two. Let me do that now.
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.
Immediately, we have "locality".
Then why doesn't this need to filter out locality
or ensure it only passes through service_name[space]
?
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.
Hmmmm ok. I do plan on using the same API that I set xDS Labels with for the locality, and even though there will be orthogonal stats handlers for GCS it will still be in same call which will get merged with this, and call out into this. So I'll add a filter here.
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.
Just talked to Yash about this, the intended behavior is to log "unknown" for the xDS labels that don't show up. Even though we want to make this general purpose, C++ actually just takes a struct here with two xDS Labels and a service_name label. Would you prefer that to avoid filtering?
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.
Discussed offline with Yash; decided to do this in the OTel layer with an additional API which we will need for GCS SetOptionalLabel, and also prepend csm. in the xDS Client.
Looks like you still need to exclude these tests from go1.20 runs, too. |
Argh yeah, I started that but forget to wrap it up/send it out. Working on it now. |
830c115
to
f3f4705
Compare
f3f4705
to
37603fd
Compare
.github/workflows/testing.yml
Outdated
@@ -103,10 +104,18 @@ jobs: | |||
go version | |||
go test ${{ matrix.testflags }} -cpu 1,4 -timeout 7m google.golang.org/grpc/... | |||
cd "${GITHUB_WORKSPACE}" | |||
set -x |
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.
Remove?
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.
Why not just keep this in to enable debugging? I'll delete though.
This PR adds support for the CSM Plugin Option, as speced in the design. The interface and the implementation live in OpenTelemetry's internal/. The next step is the usage of the CSM Plugin Option in OpenTelemetry and the global configuration.
RELEASE NOTES: N/A