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

Proposal: Move Extra packages out of this repo into a opentelemetry-rust-contrib #841

Closed
hdost opened this issue Jul 14, 2022 · 20 comments
Closed
Assignees
Labels
A-common Area:common issues that not related to specific pillar

Comments

@hdost
Copy link
Contributor

hdost commented Jul 14, 2022

Following reading more on the IdGenerator I was also taking a look at some of the other language repos, and I wonder if there shouldn't be a opentelemetry-rust-contrib repo created to hold anything outside the "core".

Updated on 2023-11-02 for consistency with the repo.

Keep in Primary:

  • opentelemetry
  • opentelemetry-http
  • opentelemetry-jaeger
  • opentelemetry-otlp
  • opentelemetry-prometheus
  • opentelemetry-proto
  • opentelemetry-sdk
  • opentelemetry-semantic-conventions
  • opentelemetry-stdout
  • opentelemetry-zipkin
  • opentelemetry-appender-log - For log which is a crate maintained by Rust Lang team
  • opentelemetry-appender-tracing - Keep for now, might be deleted or merged into tracing-opentelemetry later

Move to Contrib:

  • opentelemetry-aws
  • opentelemetry-contrib
  • opentelemetry-datadog
  • opentelemetry-dynatrace
  • opentelemetry-stackdriver
  • opentelemetry-user-events-logs
  • opentelemetry-user-events-metrics
  • opentelemetry-zpages

Additional IdGenerator implementing vendor-specific protocols such as AWS X-Ray trace id generator MUST NOT be maintained or distributed as part of the Core OpenTelemetry repositories.

Source: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#id-generators

Java: https://github.com/open-telemetry/opentelemetry-java-contrib/tree/main/aws-xray

@hdost hdost changed the title Move Extra packages out of this repo into a opentelemetry-rust-contribu Move Extra packages out of this repo into a opentelemetry-rust-contrib Jul 14, 2022
@hdost hdost changed the title Move Extra packages out of this repo into a opentelemetry-rust-contrib Proposal: Move Extra packages out of this repo into a opentelemetry-rust-contrib Jul 15, 2022
@TommyCpp
Copy link
Contributor

I think that makes sense. The discussion around this before was worried about it will be harder to maintain two separate repos.

We also publish a contrib crate that basically includes anything is not in standard but not complicate enough for a separate crate.

@TommyCpp TommyCpp added the A-common Area:common issues that not related to specific pillar label Jul 16, 2022
@djc
Copy link
Contributor

djc commented Jul 16, 2022

It's not obvious to me why Jaeger, Prometheus and Zipkin are in core and the others aren't?

@TommyCpp
Copy link
Contributor

It's not obvious to me why Jaeger, Prometheus and Zipkin are in core and the others aren't?

Those exporters are defined in spec as something SDK should support.

Another way to think about this is whether we want to maintain the model between this repo and contrib repo. Contrib repo is a great tool if some component is supported by a specific group of ppl. For example, AWS has been maintaining AWS x-ray in Java IIRC.

@hdost
Copy link
Contributor Author

hdost commented Sep 27, 2022

I think that makes sense. The discussion around this before was worried about it will be harder to maintain two separate repos.

We also publish a contrib crate that basically includes anything is not in standard but not complicate enough for a separate crate.

Something like this could make sense as well.
Would it also make sense to move it out into a separate workspace
Perhaps
<project root>/contrib

@djc
Copy link
Contributor

djc commented Sep 27, 2022

I think we should keep all the crates maintained by this org in a single workspace, so that we can benefit from centralized maintenance when core APIs change. I guess we could stuff some crates in a contrib dir, but I don't really see the point.

@hdost
Copy link
Contributor Author

hdost commented Sep 28, 2022

The most salient thing I can think of would be related to the MSRV and tying the "contribs" to the main could conflate the two.

Granted I'm sure it would be good to keep a decent pace.

Clarification that I don't mean at the crate level since thats where its set but in terms of CI.

@hdost
Copy link
Contributor Author

hdost commented Apr 12, 2023

In the SIG we spoke about delaying this until at least trace and metrics are stable.

Cc: @cijothomas @TommyCpp

@cijothomas
Copy link
Member

I understand we discussed this few months ago, but based on the discussions in the SIG meeting yesterday, want to make this happen sooner, to keep the repo only have the core-components needed to declare stable. There are too many components, and they make the CIs slow, and also take away focus from the core goal. Moving non-core to contrib repo should help keep more focus on core. (There is related topic of doing frequent, predictable release frequency as well.)

@djc
Copy link
Contributor

djc commented Nov 1, 2023

FWIW, I've come around to the point of view that there's too much stuff in this repo and that it would make sense to split it up. I guess I might now lean the other direction and say that there should be one repo per more opinionated/smaller component, rather than a kind of -contrib dumping ground, mostly because as a maintainer I'd like to not get notifications from a bunch of stuff I don't care for anyway. So I wonder how big the core set should be?

I'm guessing:

  • opentelemetry
  • opentelemetry-sdk
  • opentelemetry-semantic-conventions
  • opentelemetry-http
  • opentelemetry-proto

What else?

@cijothomas
Copy link
Member

The core - is what is defined by OTel Spec.
i.e API, SDK, Propagators (from OTel spec), Samplers, and Exporters(otlp/in-memory/zipkin/prometheus/stdout) from the Spec.

Maintainers have freedom to host more in the main repo. OTel .NET maintainers decided to host 3 instrumentation libraries in the main repo. OTel Rust can also decide to specially keep some components here. (I am particularly interested in keeping tracing-appender for now, given we already have special treatment for tracing in our api).

I might now lean the other direction and say that there should be one repo per more opinionated/smaller component

These were also discussed in many other SIGs as well (and even followed in some open-census/tracing repos). The general practice seen so far in Otel is to have a single contrib repo to begin with, and if that gets too big, then spin off further to create more dedicated repos! I think this topic is something we'd want to comeback after the initial spun up of single contrib repo.

@cijothomas
Copy link
Member

My proposed list of components to be moved out to contrib repo:

  1. opentelemetry-appender-log
  2. opentelemetry-aws
  3. opentelemetry-contrib
  4. opentelemetry-datadog
  5. opentelemetry-dynatrace
  6. opentelemetry-stackdriver
  7. opentelemetry-user-events-logs
  8. opentelemetry-user-events-metrics
  9. opentelemetry-zpages (or remove it completely)

@djc
Copy link
Contributor

djc commented Nov 1, 2023

Exporters(otlp/in-memory/zipkin/prometheus/stdout)

Why do these exporters get to stay in the core, and the others don't? That seems fairly arbitrary.

@lalitb
Copy link
Member

lalitb commented Nov 1, 2023

Why do these exporters get to stay in the core, and the others don't? That seems fairly arbitrary.

It's coming from the guidelines - https://github.com/open-telemetry/opentelemetry-specification/blob/v1.26.0/specification/library-guidelines.md#requirements

The SDK implementation should include the following exporters:

- logs, metrics, trace
OTLP (OpenTelemetry Protocol).
Standard output (or logging) to use for debugging and testing as well as an input for the various log proxy tools.
In-memory (mock) exporter that accumulates telemetry data in the local memory and allows to inspect it (useful for e.g. unit tests).
- metrics
Prometheus.
- trace
Zipkin.
Note: some of these support multiple protocols (e.g. gRPC, Thrift, etc). The exact list of protocols to implement in the exporters is TBD.

Other vendor-specific exporters (exporters that implement vendor protocols) should not be included in OpenTelemetry clients and should be placed elsewhere (the exact approach for storing and maintaining vendor-specific exporters will be defined in the future).

@hdost
Copy link
Contributor Author

hdost commented Nov 2, 2023

I've updated my description but also to note in the conversation. This should be the full list.

Keep in Primary:

  • opentelemetry
  • opentelemetry-http
  • opentelemetry-jaeger
  • opentelemetry-otlp
  • opentelemetry-prometheus
  • opentelemetry-proto
  • opentelemetry-sdk
  • opentelemetry-semantic-conventions
  • opentelemetry-stdout
  • opentelemetry-zipkin

Move to Contrib:

  • opentelemetry-appender-log
  • opentelemetry-appender-tracing
  • opentelemetry-aws
  • opentelemetry-contrib
  • opentelemetry-datadog
  • opentelemetry-dynatrace
  • opentelemetry-stackdriver
  • opentelemetry-user-events-logs
  • opentelemetry-user-events-metrics
  • opentelemetry-zpages

Side note for the execution on this what ever we do, we should not lose the history for these crates in the new repo and there should be no reason to rewrite git history.

@cijothomas
Copy link
Member

Thanks @hdost .

If okay with all, I'd like to request to keep opentelemetry-appender-tracing in the main repo for now. We are still figuring out whether that crate should be merged with tracing-opentelemetry crate, and also the potential of specially treating tracing in our logging API/SDKs. (similar to what we do for tracing!).

#1111

If we end up removing the create altogether, then its best to keep it here itself and nuke it later, as opposed to moving out to contrib and removing!

@jtescher
Copy link
Member

jtescher commented Nov 2, 2023

This change will allow better examples directories as well as we can show more usage types without the total becoming unmaintainable 👍

@TommyCpp
Copy link
Contributor

TommyCpp commented Nov 2, 2023

Should we keep opentelemetry-append-log in primary repo? log is an official component and maintained by Rust lang, I'd assume we want to keep it maintained?

@cijothomas
Copy link
Member

Should we keep opentelemetry-append-log in primary repo? log is an official component and maintained by Rust lang, I'd assume we want to keep it maintained?

I am not opposed to it. We can still keep in contrib and maintain it.

My comment about keeping appender-tracing here was more because "it is not settled that we'll need that crate or merge it into tracing-opentelemetry itself"

@hdost
Copy link
Contributor Author

hdost commented Nov 2, 2023

Added comments about the two appenders.

I've requested a new repo. In the community project. Once we have it, we should be able to progress.

@hdost hdost self-assigned this Nov 11, 2023
hdost added a commit to hdost/opentelemetry-rust-contrib that referenced this issue Nov 11, 2023
hdost added a commit to hdost/opentelemetry-rust that referenced this issue Nov 11, 2023
They have now been added in opentelemetry-rust-contrib

Relates open-telemetry#841
hdost added a commit to hdost/opentelemetry-rust that referenced this issue Nov 11, 2023
They have now been added in opentelemetry-rust-contrib

Relates open-telemetry#841

Signed-off-by: Harold Dost <h.dost@criteo.com>
@hdost hdost mentioned this issue Nov 11, 2023
4 tasks
hdost added a commit to hdost/opentelemetry-rust that referenced this issue Nov 11, 2023
They have now been added in opentelemetry-rust-contrib

Relates open-telemetry#841

Signed-off-by: Harold Dost <h.dost@criteo.com>
hdost added a commit to hdost/opentelemetry-rust that referenced this issue Nov 12, 2023
They have now been added in opentelemetry-rust-contrib

Relates open-telemetry#841

Signed-off-by: Harold Dost <h.dost@criteo.com>
hdost added a commit to hdost/opentelemetry-rust that referenced this issue Nov 13, 2023
They have now been added in opentelemetry-rust-contrib

Relates open-telemetry#841

Signed-off-by: Harold Dost <h.dost@criteo.com>
hdost added a commit to hdost/opentelemetry-rust that referenced this issue Nov 13, 2023
They have now been added in opentelemetry-rust-contrib

Relates open-telemetry#841

Signed-off-by: Harold Dost <h.dost@criteo.com>
@hdost
Copy link
Contributor Author

hdost commented Dec 22, 2023

Closing this as we now have two separate repos, and the contrib crates are moved.

@hdost hdost closed this as completed Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-common Area:common issues that not related to specific pillar
Projects
None yet
Development

No branches or pull requests

6 participants