open-telemetry / opentelemetry-java

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

Separate the SDK into separate publishable artifacts #1460

Closed kenfinnigan closed 3 years ago

kenfinnigan commented 3 years ago

Is your feature request related to a problem? Please describe. Combining the tracing and metrics API aspects into a single artifact and accessible through the same API class, OpenTelemetry, makes it difficult to utilize different solutions for metrics or tracing.

For instance, the Micrometer project is utilized by Spring Boot, Micronaut, and other Java libraries/frameworks for metrics. If they want to continue using Micrometer for metrics, but want to use tracing from OpenTelemetry, it duplicates classes and code that can cause confusion for developers.

Describe the solution you'd like I would like to see separate artifacts for tracing and metrics that can be consumed individually if projects so desire.

There can still be a separate artifact that includes the all-encompassing OpenTelemetry for those developers who want to utilize both aspects of OpenTelemetry.

jkwatson commented 3 years ago

Nothing in the current design precludes a user from just using the tracing APIs (or the metric APIs) and ignoring the other. I don't think this would provide much value for the user, and would definitely complicate things in the project itself.

kenfinnigan commented 3 years ago

Would you not consider it a bad developer experience to have metrics APIs present and expect no one to use them? In addition to packaging code that adds size without value if not used?

To me, that seems counter-intuitive.

jkwatson commented 3 years ago

I don't think it's a big deal for developers, no. The API codebase is quite small, and if you don't use the classes, you don't load the classes. And, for users who want to use both APIs (and logging, in the future), it definitely is more of a pain to have to figure out which of the API modules to depend on.

One of the goals of the OpenTelemetry project is to have a single API for all the standard telemetry signals. I think that separating the APIs into separate modules would defeat that goal. Even if we did it, there would be the 3rd module with all the common code in it which both would have to depend on. I think this would add more confusion that would be reduced by separating the APIs.

jkwatson commented 3 years ago

I'll also let @carlosalberto and @bogdandrutu chime in on this one. My opinion is only one of many. :)

carlosalberto commented 3 years ago

Same feeling as @jkwatson - the API is very small and we indeed want to offer a unified experience (if possible).

kenfinnigan commented 3 years ago

That's fair.

Looking at it from a different direction, could the sdk be split instead?

I'm currently looking at how to use Micrometer with the OpenTelemetry Metrics APIs, and it would be nice if from an implementation side I could plug in Micrometer for the Metrics piece, but still use the OpenTelemetry SDK for everything else.

Maybe split into:

jkwatson commented 3 years ago

This should be possible right now, if you provide an SPI provider just for the metrics SDK Provider, this should work as-is. If there is something that won't work with that approach, I definitely want to fix that!

kenfinnigan commented 3 years ago

Working on that so can't comment directly at present.

But I was thinking of it from the perspective of plugging in your own provider, but then not having the SDK metrics provider present at all.

With Quarkus there is a lot of work done to minimize the number of classes being brought along with an application, whether executing on a JVM or building an image with native, which is easier if an entire dependency could be excluded, for instance, rather than relying on dead code elimination to remove it.

jkwatson commented 3 years ago

Ah! This is very good to know. Yes, I think having separate artifacts would be very useful for this reason. I think this will be a bit of work to properly untangle, so we probably won't get to it until after GA (unless you have time to do it, of course!). Can you rewrite this issue (or make a separate issue..your call) to publish independent SDK artifacts?

kenfinnigan commented 3 years ago

I've modified the title, let me know if that fits better.

I might try and tackle it next week, depending on how things go

jkwatson commented 3 years ago

@kenfinnigan I'm marking this as "after-ga" only to signify that we could go GA without it, not that it absolutely shouldn't be done.

kenfinnigan commented 3 years ago

Not a problem

kenfinnigan commented 3 years ago

@jkwatson could you assign the issue to me, I'll be working on it over the coming days.

Any particular "gotchas" I should be aware of when splitting the bits out?

My current idea is to split it into:

Does that seem reasonable?

kenfinnigan commented 3 years ago

I'm having a few issues figuring out the right project structure, as I've not used Gradle in the past.

I originally tried:

sdk/
  common/
  correlation_context/
  metrics/
  sdk/
  tracing/

But that has issues finding the "complete" sdk project, given I added .replace("opentelemetry-sdk-", "sdk/") into settings.gradle.

Would a better approach be to have:

sdk-parts/
  common/
  correlation_context/
  metrics/
  tracing/
sdk/

sdk-parts is just an arbitrary name, could be anything.

Any thoughts/feedback appreciated

Oberon00 commented 3 years ago

But that has issues finding the "complete" sdk project, given I added .replace("opentelemetry-sdk-", "sdk/") into settings.gradle.

You can just write project('opentelemetry-sdk').projectDir = "$rootDir/sdk/sdk" as File at the end of settings.gradle. Another possibility would be to have the sdk directly in sdk/ (i.e. have a sdk/build.gradle and sdk/src/main/gradle, etc alongside the subprojects). I'm not sure how conventional and well-supported that is though.

jkwatson commented 3 years ago

I haven't thought a huge amount about this, but maybe

sdk/
  common/
  correlation_context/
  metrics/
  all/
  tracing/

With the sdk/all directory published with the same name as the sdk is today (opentelemetry-sdk, I believe) ?

kenfinnigan commented 3 years ago

Thanks, I will go with all/ and use @Oberon00's trick for in settings.gradle

bogdandrutu commented 3 years ago

This should be discussed in the specs, because this was one of the main point of having all telemetry together

jkwatson commented 3 years ago

This should be discussed in the specs, because this was one of the main point of having all telemetry together

We would still be publishing an artifact with everything together. Just provide an option for people who only would like to implement (for example) an alternate metrics SDK, and still be able to use tracing as-is.

bogdandrutu commented 3 years ago

The problem is that you need to use boms otherwise will be hard to ensure all the versions are the same for all artifacts, and will cause troubles for others. That being said I think we can ship a trace/metrics/etc and a SDK package. But would still like to hear specs opinion on this

kenfinnigan commented 3 years ago

Not sure I see why boms are needed if all artifacts are released at the same time. Won't they always be in sync?

@bogdandrutu @jkwatson what's the appropriate way to get the opinion from those in the spec group?

jkwatson commented 3 years ago

Not sure I see why boms are needed if all artifacts are released at the same time. Won't they always be in sync?

A BOM would make it easier for users to keep things in sync from the consumer side. Not strictly required, but definitely nicer for the end consumer.

@bogdandrutu @jkwatson what's the appropriate way to get the opinion from those in the spec group?

The first step would be to raise an issue in the specification repo about it.

kenfinnigan commented 3 years ago

Not sure I see why boms are needed if all artifacts are released at the same time. Won't they always be in sync?

A BOM would make it easier for users to keep things in sync from the consumer side. Not strictly required, but definitely nicer for the end consumer.

Should I be adding an SDK bom as part of this PR?

@bogdandrutu @jkwatson what's the appropriate way to get the opinion from those in the spec group?

The first step would be to raise an issue in the specification repo about it.

I'll raise an issue

ebullient commented 3 years ago

The spec is about the API, certainly, not the SDK (which is an implementation of the API). While perhaps considered unlikely, there is always the possibility that someone would make their own SDK wholesale (and this isn't against the spec, as the spec is focused on the API and the responsibility of the SDK, not the SDK itself). Making the SDK more modular (which would allow some customization of how the SDK works), doesn't seem in any way counter to the spec. The spec is still one entity. What am I missing?

Oberon00 commented 3 years ago

There is a spec for the SDK too, but the spec is also modularized in trace/metric/resource/..., so I also don't think this runs counter to the spec.

jkwatson commented 3 years ago

@kenfinnigan If you can look into adding a BOM, that would be great. Otherwise, we can make it a follow-up issue to get implemented.