open-telemetry / opentelemetry-java

OpenTelemetry Java SDK
https://opentelemetry.io
Apache License 2.0
1.99k stars 829 forks source link

Move non-core components out of opentelemetry-java #4661

Closed jack-berg closed 2 years ago

jack-berg commented 2 years ago

Following the spirit of moving opentelemetry-extension-annotations to a more appropriate home in opentelemetry-java-instrumentation, and prompted by a discussion in the 7/28 Office Hours, I've done a scan of the project for components that are candidates to live elsewhere. The list includes all artifacts that contain code that isn't explicitly included in the spec, with exceptions for opentelemetry-micrometer1-shim and opentelemetry-extension-kotlin.

Artifact Status Description
opentelemetry-extension-aws stable Consists of AwsXRayPropagator and AwsConfigurablePropagator, which implements the ConfigurablePropagatorProvider SPI.
opentelemetry-extension-incubator alpha Consists of PassThroughPropagator and ExtendedTracer.
opentelemetry-extension-noop-api alpha Includes a completely noop OpenTelemetry implementation.
opentelemetry-sdk-extension-resources stable Resource detectors for container, host, os, and process
opentelemetry-sdk-extension-aws stable AWS resource providers.
opentelemetry-sdk-extension-jfr-events alpha SpanProcessor which records spans as JFR events.
opentelemetry-sdk-extension-metric-incubator alpha Includes file based view configuration.
opentelemetry-sdk-extension-tracing-incubator alpha Includes the LeadDetectingSpanProcessor
opentelemetry-sdk-extension-zpages alpha zPages SpanProcessor

Comments

anuraaga commented 2 years ago

I'd like to see this deprecated and moved to opentelemetry-java-contrib/aws-xray.

Just for some history, actually the name of the propagator isn't really correct but stuck unfortunately - the propagator isn't really related to x-ray (besides the x-ray sdk defaulting to it which is probably where the name came from), but it's the propagation format used within aws. Notably, many propagation scenarios require it regardless of tracing backend. So aws-xray would not be a good artifact for it, which is for x-ray backend specific code.

Also, as this is already used in instrumentation, if moving it to contrib, it then requires deciding on the pattern for depending on contrib from the instrumentation repo. Compared to the others on the list, this one is actually probably being used the most in production by users, and I suspect most aren't x-ray users :P

jack-berg commented 2 years ago

This text from the spec seems pretty unambiguous about this propagator not belonging in the core repo:

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

if moving it to contrib, it then requires deciding on the pattern for depending on contrib from the instrumentation repo.

Yes, we need to decide this :)

willarmiros commented 2 years ago

Hi @jack-berg, would moving these components to a new home change their artifact name in Maven?

jack-berg commented 2 years ago

Yes. And for stable artifacts we would have to continue to publish the old coordinates until a 2.0.0 release. Alpha artifacts don't have the same restriction, and if moved would likely be published in both places for one version (while deprecated in opentelemetry-java) before being removed.

mateuszrzeszutek commented 2 years ago

I think that the AWS Xray propagator should probably stay in the SDK; and that we should change the spec to reflect that. I view it as a component that is similar in nature to the OpenTracing shim, Micrometer shim, Jaeger propagator, etc. in that it enables OpenTelemetry to talk to legacy systems, and thus makes the transition to OTel (and W3C propagation) smoother. Perhaps it should appear in the MAY be maintained list next to the OTTrace propagator: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/context/api-propagators.md#propagators-distribution

trask commented 2 years ago
  • opentelemetry-extension-aws - Not sure what the history is here, but xray propagator isn't included in the list of propagators defined the spec and including it in the core sets a precedent of making exceptions. I'd like to see this deprecated and moved to opentelemetry-java-contrib/aws-xray.

+1 to @mateuszrzeszutek's comment. And if this change is made to the specification, it would probably make sense to move the aws xray propagator to https://github.com/open-telemetry/opentelemetry-java/tree/main/extensions/trace-propagators, though that would change their its coordinates.

  • opentelemetry-extension-incubator - I like the idea of having places to incubate API / SDK concepts, but think if they're going to live in the core project there needs to be a path to inclusion the stable API / SDK artifacts. Not clear that that path exists for PassThroughPropagator or ExtendedTracer.

PassThroughPropagator seems interesting, I could see that maybe being spec'd someday

I don't see much of a path forward for ExtendedTracer either, I'm not sure it makes a lot of sense in contrib either, so I'd suggest just dropping it and if anyone notices then we've found a potential maintainer for it in contrib πŸ˜„

  • opentelemetry-extension-noop-api - Should probably move this toopentelemetry-java-contrib. If we think its important enough to keep in core, perhaps opentelemetry-extension-incubator, since there may be a chance for a completely noop implementation to be added to the spec some day.

πŸ‘ for opentelemetry-extension-incubator

  • opentelemetry-sdk-extension-resources - I don't feel especially strongly about this, but this probably makes more sense in opentelemetry-java-instrumentation.

yeah, this is interesting, I hadn't thought about it previously, but I think keeping semantic convention stuff out of the core repo is very reasonable (since for Java we have a dedicated "instrumentation" repo). this could include the semconv artifact too as we discussed in this week's SIG meeting. we can update https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/docs/scope.md if we move forward with this plan.

  • opentelemetry-sdk-extension-aws - I think this makes most sense in opentelemetry-java-instrumentation, especially if the standard resource providers end up there.

πŸ‘

  • opentelemetry-sdk-extension-jfr-events - Move to opentelemetry-java-contrib.

πŸ‘ (I'm ok looking for component owner for this either before or after the move)

  • opentelemetry-sdk-extension-metric-incubator - Can probably retain since file based SDK configuration is being very seriously discussed in the specification.

πŸ‘

  • opentelemetry-sdk-extension-tracing-incubator - If we're confident enough with the LeakDetectingSpanProcessor, we should consider promoting it to the stable opentelemetry-sdk-testing artifact, since it makes sense as a testing utility.

LeakDetectingSpanProcessor feels different to me from the opentelemetry-sdk-testing artifact which is designed around JUnit / AssertJ. although I don't have a better suggestion though, other than contrib

  • opentelemetry-sdk-extension-zpages - zPages are mentioned in the experimental section of the specification, but I think this could be merged into opentelemetry-sdk-extension-tracing-incubator, since there are no transitive dependencies required to run.

πŸ‘

rapphil commented 2 years ago

Yes. And for stable artifacts we would have to continue to publish the old coordinates until a 2.0.0 release. Alpha artifacts don't have the same restriction, and if moved would likely be published in both places for one version (while deprecated in opentelemetry-java) before being removed.

@jack-berg besides maven coordinates, would package names remain the same during the transition?

jack-berg commented 2 years ago

@rapphil it depends on where they move. Artifacts in opentelemetry-java have a package prefix of io.opentelemetry. Artifacts in opentelemetry-java-instrumentation have a package prefix of io.opentelemetry.instrumentation. Artifacts in opentelemetry-java-contrib have a package prefix of io.opentelemetry.contrib. So a move to a different repository would have a package name change. A move within this repository as I've proposed with some of the artifacts may or may not have a package name change.

trask commented 2 years ago

Should micrometer shim be moved out?

And settle on criteria for core repo: spec'd (or on its way to spec'd) AND not semantic convention related

jack-berg commented 2 years ago

On the subject of the micrometer shim:

I think we should be stricter and exclude components from here unless they are explicitly spec'd. I know we made an exception for micrometer. And that seemed like the right decision at the time, but micrometer tracing doesn't share the same usage and yet a good user experience requires that it belongs next to the micrometer metrics shim.

I believe that opentelemetry-java-instrumentation is the right home for both the micrometer metrics and tracing shim. These components are conceptually similar to the kafka metrics bridge, the logback appender, and the log4j appender. Each of these components bridges some other telemetry API into the opentelemetry API. And each is related to very popular java specific libraries that are unlikely to be part of the specification.

We could take it one step further and move even the spec'd shims to opentelemetry-java-instrumentation (opentracing and opencensus). That has the added benefit of having all the shims in one place, whether or not they are explicitly spec'd.

trask commented 2 years ago

My preference would be to move "bridges to other telemetry libraries" to the contrib repository, where experts in the target libraries can be component owners for them.

trask commented 2 years ago

We could probably reverse the dependency between instrumentation and contrib repos, making instrumentation repo depend on contrib repo. We have already discussed moving runtime attach and static instrumenter to the instrumentation repo, which I think are the only contrib components that depend on the instrumentation repo today.

mateuszrzeszutek commented 2 years ago

We could take it one step further and move even the spec'd shims to opentelemetry-java-instrumentation (opentracing and opencensus).

One possible problem with the OpenCensus bridge is that it's impossible to use it together with the javaagent, since it's an SDK bridge. Maybe it's just me, but it would be a bit strange to have it in the javaagent repo when it's impossible to use it together with the javaagent.

We could probably reverse the dependency between instrumentation and contrib repos, making instrumentation repo depend on contrib repo.

This is a pretty good idea πŸ‘

jack-berg commented 2 years ago

My preference would be to move "bridges to other telemetry libraries" to the contrib repository, where experts in the target libraries can be component owners for them.

Doesn't the instrumentation repo depend on library experts chiming in from time to time as well? I think we should use this as an opportunity to really clarify the delineation between core, instrumentation, and contrib. I think we're coalescing on consensus that core only contains spec'd components which don't involve the semantic convention. I'm not as confident in the difference between contrib and instrumentation. Maybe contrib is the place for components that aren't spec'd, and don't involve the semantic convention. This definition would mean that contrib becomes the home for the micrometer metrics & tracing shims, as well as the kafka client metrics stuff.

One possible problem with the OpenCensus bridge is that it's impossible to use it together with the javaagent, since it's an SDK bridge. Maybe it's just me, but it would be a bit strange to have it in the javaagent repo when it's impossible to use it together with the javaagent.

Due to the presence of a growing number of library instrumentation artifacts, I've been viewing the instrumentation repo as a general home for instrumentation, which also publishes an agent which automatically weaves lots of instrumentation into apps at runtime. I think you're suggesting that instrumentation which can't be used by the agent should live in contrib, which seems like a strange delineation to me.

trask commented 2 years ago

Due to the presence of a growing number of library instrumentation artifacts, I've been viewing the instrumentation repo as a general home for instrumentation

This is correct, and has been the case since we renamed the repo from opentelemetry-java-auto-instr to opentelemetry-java-instrumentation a long while back.

I think you're suggesting that instrumentation which can't be used by the agent should live in contrib

I don't believe @mateuszrzeszutek is suggesting this, as he has been one of the people pushing the instrumentation repo forward for library (non-javaagent) instrumentation.

mateuszrzeszutek commented 2 years ago

Due to the presence of a growing number of library instrumentation artifacts, I've been viewing the instrumentation repo as a general home for instrumentation, which also publishes an agent which automatically weaves lots of instrumentation into apps at runtime. I think you're suggesting that instrumentation which can't be used by the agent should live in contrib, which seems like a strange delineation to me.

I was just thinking out loud; you're right that it seems kinda strange.

I think that the decision here might just boil down to deciding the difference between the instrumentation and contrib repos:

I'm not as confident in the difference between contrib and instrumentation. Maybe contrib is the place for components that aren't spec'd, and don't involve the semantic convention. This definition would mean that contrib becomes the home for the micrometer metrics & tracing shims, as well as the kafka client metrics stuff.

The instrumentation repo already contains a bunch of features/components that are not specced and/or experimental, and I think it's been mostly working fine for it. The contrib repo contains plenty of instrumentations, if we define an instrumentation as "something that produces telemetry" - for example, the maven extension, which I believe is in its right place and I don't think I'd like to move it to the instrumentation repo. Also, some time ago we've been thinking about moving the less important/popular/supported instrumentations over to the contrib repo - and if we reverse the dependency between contrib and instrumentation we'll finally be able to do that.

How about we define the instrumentation repo as: instrumentation artifacts that are supported by OpenTelemetry maintainers. Which sounds super vague, but I think somewhat makes sense - we'd like to support things like OpenCensus and OpenTracing, or the most popular Java frameworks (servlet, spring, micrometer, ...). So, for example, Kafka instrumentation would stay instrumentation; Apache Dubbo or RocketMQ would be moved to contrib and we'd have to find owners for them (we have no idea how they work anyway πŸ˜‚ ). Everything that does not fit that definition goes to contrib.

(This definition kinda contradicts the maven extension example - maven is super popular, but I'd argue that instrumenting build systems is not so for the time being its place is in contrib)

jack-berg commented 2 years ago

Which sounds super vague, but I think somewhat makes sense

Ideally the definition we land on is unambiguous so we don't have to keep revisiting the decision in the future. I think your idea of "instrumentation artifacts that are supported by OpenTelemetry maintainers" could be made unambiguous by defining a process for maintainer support - if no maintainer is willing to "sponsor" the instrumentation, its a candidate to move to contrib. Still a bit tenuous.

On the note of opentelemetry-extension-aws aws propagator, we discussed the spec issue today in spec SIG. The consensus was pretty much to treat it as a bug in the java implementation, calling it out in the readme / an issue that this was a mistake. But the conversation was interesting because there was also consensus that we don't need to keep publishing stable artifacts to be compliant with semantic versioning. As long as the we don't break the API those components depend, folks can use an old version of a deprecated artifact with the latest API / SDK artifacts. We've been operating under this assumption with jaeger-proto, extension-annotations,opentelemetry-extension-aws, opentelemetry-sdk-extension-resources. Changing that assumption would open up the door for more options for aligning components with their proper long term home. Folks in the spec SIG noted that there are reasons we might want to continue maintaining deprecated code, such as to provide fixes for security vulnerabilities. But we don't need to necessarily keep publishing artifacts to do so, since we could provide patches to release branches.

trask commented 2 years ago

Here's a proposal based on the above:

Then the javaagent published from the instrumentation repo would only contain components that are "sponsored" by a maintainer (either via the core repo or the instrumentation repo), while the javaagent published from the contrib repo would be equivalent to the javaagent that we publish currently.

While this definition of having a maintainer willing to "sponsor" a component is wishy-washy, it's not clear to me that we're going to come up with anything better.

jack-berg commented 2 years ago

We spoke in the 8/25/2022 Java SIG about a variety of topics related to where components should live within the three otel java repositorys opentelemetry-java, opentelemetry-java-instrumentation, and opentelemetry-java-contrib. To summarize:

Changes to this repository as a result of this discussion include:

We still need to make a decision on:

trask commented 2 years ago
  • What to do with opentelemetry-extension-noop-api. We like the idea of moving it to opentelemetry-extension-incubator, but after some research that's not actually a good idea. The problem is it implements the ContextStorageProvider SPI, which means that if its included on the classpath the noop context storage provider will get used automatically. If we move to opentelemetry-extension-incubator, users will find context mysteriously not working. It has to either stay where it is, or move to opentelemetry-java-contrib. I'm in favor of moving to opentelemetry-java-contrib.

πŸ‘ can always move it to core repo if it gets specd some day

guizmaii commented 2 years ago

Hi,

After the move of the noop-api to io.opentelemetry.contrib, I don't find it in Maven Central:

Screen Shot 2022-09-11 at 12 45 46 pm

See https://search.maven.org/search?q=io.opentelemetry.contrib

Where can I find it?

Thanks πŸ™‚

jack-berg commented 2 years ago

The artifact was moved to opentelemetry-java-contrib/noop-api. There's a one week gap between the release of opentelemetry-java and opentelemetry-java-contrib. The noop-api will be first published this coming Friday, Sept 16th with maven coordinates io.opentelemetry.contrib:opentelemetry-noop-api:{version}.

guizmaii commented 2 years ago

Hi @jack-berg,

Is the io.opentelemetry.contrib:opentelemetry-noop-api being published already? I still don't find it in Maven Central

mateuszrzeszutek commented 2 years ago

Hey @guizmaii , We haven't released the 1.18.0 version of contrib yet, we've had an unexpected delay, sorry for that πŸ™ You can expect it to be released early this week.

jkwatson commented 2 years ago

@guizmaii this has now been released, so should be available. I have verified it can be downloaded via those maven coordinates.

jack-berg commented 2 years ago

All items except opentelemetry-extension-aws have been addressed. Moving that conversation to #4831 and closing this issue.