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 |
zasweq
force-pushed
the
csm-plugin-option-add
branch
from
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.
Why don't we merge these labels externally, rather than passing them in here and merging them?
There was a problem hiding this comment.
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.
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.
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.
Oh whoops I didn't prepend csm. for the two. Let me do that now.
There was a problem hiding this comment.
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.
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.
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.
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. |
zasweq
force-pushed
the
csm-plugin-option-add
branch
from
830c115
to
f3f4705
Compare
zasweq
force-pushed
the
csm-plugin-option-add
branch
from
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.
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