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

Consider making stats/opentelemetry its own module #8092

Open
dashpole opened this issue Feb 14, 2025 · 3 comments
Open

Consider making stats/opentelemetry its own module #8092

dashpole opened this issue Feb 14, 2025 · 3 comments
Assignees
Labels
Area: Observability Includes Stats, Tracing, Channelz, Healthz, Binlog, Reflection, Admin, GCP Observability Type: Dependencies Updating/adding/removing dependencies

Comments

@dashpole
Copy link

Please see the FAQ in our main README.md before submitting your issue.

Use case(s) - what problem will this feature solve?

We (kubernetes) were surprised to see dependencies on go.opentelemetry.io/contrib/detectors/gcp brought in with the latest gRPC release, despite not using that OTel detector.

Proposed Solution

Create a separate go module for stats/opentelemetry, so only users of that library pull in its dependencies.

Alternatives Considered

Additional Context

kubernetes/kubernetes#128919 (comment)

@dashpole dashpole added the Type: Feature New features or improvements in behavior label Feb 14, 2025
@arjan-bal arjan-bal added Type: Dependencies Updating/adding/removing dependencies Area: Observability Includes Stats, Tracing, Channelz, Healthz, Binlog, Reflection, Admin, GCP Observability and removed Type: Feature New features or improvements in behavior labels Feb 17, 2025
@arjan-bal arjan-bal self-assigned this Feb 17, 2025
@arjan-bal
Copy link
Contributor

This is the PR that merged the openetelemetry module into the main grpc-go module: #7759

There is a discussion about a similar concern of introducing a gcp specific dependency: #7759 (comment)

Quoting from the discussion:

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).

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.

@arjan-bal
Copy link
Contributor

@dashpole does this address your concern? If not, I can discuss this with other maintainers and get back.

@liggitt
Copy link

liggitt commented Feb 19, 2025

Unfortunately, it does not. Binary size is important, but even if the dependencies aren't linked, they still contribute to the dependency graph and get passed to all downstreams, and contribute to and impose constraints on the dependency versions selected by go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Observability Includes Stats, Tracing, Channelz, Healthz, Binlog, Reflection, Admin, GCP Observability Type: Dependencies Updating/adding/removing dependencies
Projects
None yet
Development

No branches or pull requests

3 participants