opentelemetry-rust icon indicating copy to clipboard operation
opentelemetry-rust copied to clipboard

Proposal: Rename opentelemetry-proto/src/proto/tonic module

Open psandana opened this issue 10 months ago • 8 comments

As mentioned in https://github.com/open-telemetry/opentelemetry-rust-contrib/pull/134#issuecomment-2587606751, OpenTelemetry use proto defined structs from this tonic module. Despite the name references Tonic, they are pretty generic, not restricted by Tonic scenarios.

Thus, I propose to rename the module to better describe the contents.

https://github.com/open-telemetry/opentelemetry-rust/tree/main/opentelemetry-proto/src/proto/tonic

The first obvious option to me, is just to place the proto/tonic/* files directly in the proto/ folder. This more-or-less follows the name they got from the upstream opentelemetry-proto repo.

psandana avatar Jan 14 '25 15:01 psandana

@psandana - Agree with the change. Moving directly to proto/ dir, or else to src/generated dir ( so src contains proto, generated and transform dirs). And I think it would make sense to keep it non-breaking, by still keeping the tonic as an alias?

lalitb avatar Jan 17 '25 19:01 lalitb

If you prefer to keep tonic as an alias, I think it is ok. Why is the reason to keep it as is? If you still plan to remove, just not now, I would suggest adding a deprecation notice.

psandana avatar Jan 17 '25 19:01 psandana

Why is the reason to keep it as is? If you still plan to remove, just not now, I would suggest adding a deprecation notice.

Yes, want to deprecate and then remove.

lalitb avatar Jan 17 '25 19:01 lalitb

FYI, there is a feature gen-tonic-messages. Shall this be renamed also?

psandana avatar Jan 17 '25 19:01 psandana

@psandana Sorry for late reply.

FYI, there is a feature gen-tonic-messages. Shall this be renamed also?

Yes, that can be renamed. However, the gRPC code for services within tonic directory will depend on tonic, and we would still need a feature flag containing "tonic" for that.

lalitb avatar Feb 26 '25 00:02 lalitb

Also, to add the historical reason behind the existence of tonic directory - earlier both tonic + prost and grpcio + protobuf were supported for generating code, and so there was specific directories for tonic and grpcio. The grpcio code generation was later removed. And then we were left with tonic directory :)

lalitb avatar Feb 26 '25 00:02 lalitb

So the change is more complex than expected :D granted. Let see if we can do something meaningful about this during the next weeks.

psandana avatar Mar 11 '25 19:03 psandana

My proposal to have cleaner design:

  • All Protobuf messages are currently generated under src/proto/tonic. These should be restructured (logically, not physically) into proto::opentelemetry::proto::*

  • All gRPC service code (ie generated files with naming *collector*) can remain in the same directory (src/proto/tonic) but be exposed via proto::tonic::opentelemetry::proto::collector::*, behind the gen-tonic feature.

  • (optional) To reduce confusion about the "tonic" directory containing non-tonic code, we could consider renaming src/proto/tonic to something more neutral like src/proto/generated, while still organizing tonic-specific exports via a mod tonic gate. This keeps file moves minimal while improving clarity.

  • gen-tonic-messages feature flag to be removed completely. This was only required when both tonic + prost and grpcio + protobuf were used, which is no longer a case.

This allows users who only need OTLP types for serialization/deserialization (e.g., over HTTP or file) to avoid unnecessary dependencies like tonic, keeping builds lean and dependency trees minimal.

Also, as a separate improvement: opentelemetry-proto should not depend on opentelemetry-sdk. Instead, the conversion logic (from SDK data types to OTLP wire format) could live in opentelemetry-otlp — keeping opentelemetry-proto focused purely on data definitions.

I have some idea on how to do this cleanup - can pick this sometime next week, if no one has concern, or looking into this.

lalitb avatar Apr 01 '25 23:04 lalitb

closing this, as this will be covered through #3045.

lalitb avatar Jun 30 '25 16:06 lalitb