-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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: Remove OpenTelemetry module and add RLS Metrics e2e tests #7759
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7759 +/- ##
==========================================
+ Coverage 81.43% 81.73% +0.30%
==========================================
Files 368 374 +6
Lines 36799 38136 +1337
==========================================
+ Hits 29967 31171 +1204
- Misses 5606 5691 +85
- Partials 1226 1274 +48 |
5f8f849
to
965d55f
Compare
@@ -10,6 +10,11 @@ require ( | |||
github.com/golang/protobuf v1.5.4 | |||
github.com/google/go-cmp v0.6.0 | |||
github.com/google/uuid v1.6.0 | |||
go.opentelemetry.io/contrib/detectors/gcp v1.31.0 |
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.
It would be great to avoid adding any cloud-specific dependencies. Why not let users inject detectors?
Hm, actually, looking at constructMetadataFromEnv()
- it's not the job of gRPC (or any other) library to get those labels, right? resource.New()
should be called by the application to centrally construct the resource (which represents the application). Other metrics, traces, etc all need the resource too:
// Resource describes an entity about which identifying information
// and metadata is exposed. Resource is an immutable object,
// equivalent to a map from key to unique value.
//
// Resources should be passed and stored as pointers
// (`*resource.Resource`). The `nil` value is equivalent to an empty
// Resource.
type Resource struct {
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.
If I use gcp detector in my app, I'll have duplicate attributes. OTEL will probably filter them out, but that's a waste of resources.
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.
Adding this dependency doesn't affect your detectors right? This dependency is part of the CSM Observability feature, and doesn't get invoked unless that feature is explicitly configured.
From my understanding, the detectors are entirely orthogonal unless you chain results together, and are based off environment variables and properties. Thus, this doesn't set any properties of the environment and only optionally reads it if CSM Observability is turned on, and thus shouldn't affect your own detectors.
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.
Ok, I see CSM is Google Cloud Service Mesh
. It makes sense to hard-code the GCP detector then.
Still, would be better for your consumers to have this dependency as an opt-in (most people don't need it). Perhaps the csm
package can be made a separate module? grpc-go is used in so many projects. I think it makes sense to minimize the number of dependencies that are brought 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.
The decision was made to release these CSM Observability/OpenTelemetry API's as stable. Rather than have OpenTelemetry as it's own module, we decided to release these features as simply part of our top level go module, as C-Core does. So I think this will be left here. Note that this is simply listing the dependencies. It does not affect binary size or performance unless the feature that uses the dependency is enabled (in this case, CSM Observability).
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.
Ok, thank you!
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.
Note that we also put in some safeguards to help protect this from accidentally getting imported into any of the main grpc packages in #7766. There are some nasty ways they could sneak in by moving common functionality to an internal
package and then adding an internal dependency on that from somewhere else. We have similar concerns about other things affecting binary size and dependency trees like xds.
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.
That's great, thank you!
Redid #7655 as dependencies were updated on master.
This PR adds e2e tests for RLS Metrics, with verifications of the OpenTelemetry metrics atoms emitted.
RELEASE NOTES: