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

Split the otlp module into multiple submodules #183

Closed
wants to merge 18 commits into from

Conversation

dmathieu
Copy link
Member

@dmathieu dmathieu commented Jul 23, 2024

This is a proposal to split the otlp module into multiple submodule.
That way, requiring otlp/trace does not add google.golang.org/grpc as a dependency.

This should allow us to deprecate the slim module.
This would be a first path towards open-telemetry/opentelemetry-go#2579

Technically, this does multiple things:

  • Upgrade CI to 1.21, since 1.20 has reached EOL.
  • Create modules for each subfolder in otlp/.
  • Create replace directives for each of those modules.
  • Run go mod tidy in each of those modules.
  • Run crosslink in each of those modules (to remove replace directives we don't need).

This also removes the root go.mod and go.sum. That module has zero code, so it can't be imported and there's no reason to maintain a module there (keeping them also caused crosslink issues).

I'm happy to split this PR into multiple smaller ones if we want.

@dmathieu dmathieu force-pushed the split-modules branch 4 times, most recently from 6704fbc to 4b3a462 Compare July 23, 2024 09:33
@dmathieu dmathieu marked this pull request as ready for review July 23, 2024 12:16
@dmathieu dmathieu requested a review from a team July 23, 2024 12:16
go.opentelemetry.io/proto/otlp/metrics v0.0.0-00010101000000-000000000000
go.opentelemetry.io/proto/otlp/profiles v0.0.0-00010101000000-000000000000
go.opentelemetry.io/proto/otlp/trace v0.0.0-00010101000000-000000000000
google.golang.org/grpc v1.65.0
Copy link
Member

Choose a reason for hiding this comment

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

The OTLP HTTP exporters are depending on go.opentelemetry.io/proto/otlp/collector.
E.g. from otlptracehttp: https://github.com/open-telemetry/opentelemetry-go/blob/d4b5396f848504859ba50946e4724b84b71b36a5/exporters/otlp/otlptrace/otlptracehttp/client.go#L128
Therefore all actual OTLP exporters will still depend on google.golang.org/grpc and it will not help with solving open-telemetry/opentelemetry-go#2579.

Copy link
Member

@pellared pellared Jul 24, 2024

Choose a reason for hiding this comment

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

I think the possible solutions are:

  1. solving otlp*http modules import grpc opentelemetry-go#2579 (comment) in google.golang.org/protobuf (so that slim would work without issues) (mentioned also here)
  2. in go.opentelemetry.io/proto/otlp/collector split protobuf messages and gRPC services code from into separate packages and modules (this would be a path more aligned with current proposal)

Personally, I would be in favor of trying the option 1 first.

Copy link
Member

Choose a reason for hiding this comment

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

PS. This was my first idea when I was trying to fix open-telemetry/opentelemetry-go#2579. And I created slim because of #183 (comment). At that point of time I was totally not aware of https://protobuf.dev/reference/go/faq/#namespace-conflict.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right about the need for the collector even in HTTP, sorry.
I'll need to investigate more, but this PR, alongside a new collectorslim module which only includes the collector slim, and reuses the other data types may solve our registration issue?

Copy link
Member

@pellared pellared Jul 25, 2024

Choose a reason for hiding this comment

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

but this PR, alongside a new collectorslim module which only includes the collector slim, and reuses the other data types may solve our registration issue?

It could solve the issue. Probably this would require more post-processing after protoc code generation.

However, I would be more in favor of checking if it is possible to solve somehow https://protobuf.dev/reference/go/faq/#namespace-conflict. Then we could simply use the slim modules in OTLP HTTP exporters. Moreover, it can also help others dealing with the namespace conflict.

@dmathieu dmathieu closed this Aug 22, 2024
@dmathieu dmathieu deleted the split-modules branch August 22, 2024 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants