Promote ComponentLoader to new opentelemetry-api-util, standardize SPI loading

[jack-berg]

Related to #7292, #7181, and convo in #7164.

Promote ComponentLoader to opentelemetry-common, a new artifact with common utilities shared between opentelemetry-api and opentelemetry-context.

Extend ConfigProperties, DeclarativeConfigProperties with new getComponentLoader method, allowing any component participating in configuration to use the ComponentLoader configured via AutoConfiguredOpenTelemetrySdkBuilder#setComponentLoader (i.e. like the one configured in the agent and spring boot starter).

Update all OTLP exporter SPI implementations to use the component loader from ConfigProperties / DeclarativeConfigProperties to load various OTLP SPIs including HttpSenderProvider, GrpcSenderProvider, and CompressorProvider (once #7428 is merged).

[codecov]

Codecov Report

Attention: Patch coverage is 97.75281% with 2 lines in your changes missing coverage. Please review.

Project coverage is 89.95%. Comparing base (915c64a) to head (da46069).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
...emetry/sdk/autoconfigure/spi/ConfigProperties.java	0.00%	1 Missing ⚠️
...nfigure/AutoConfiguredOpenTelemetrySdkBuilder.java	75.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #7446      +/-   ##
============================================
- Coverage     89.96%   89.95%   -0.01%     
- Complexity     7034     7042       +8     
============================================
  Files           799      801       +2     
  Lines         21256    21296      +40     
  Branches       2061     2061              
============================================
+ Hits          19122    19157      +35     
- Misses         1479     1481       +2     
- Partials        655      658       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[trask]

Promote ComponentLoader to opentelemetry-context

why not opentelemetry-sdk-common?

[jack-berg]

why not opentelemetry-sdk-common?

Couple reasons:

• Future proof. Currently ContextStorageProvider is loaded via SPI, and although the ClassLoader / ComponentLoader used to load it is not configurable, its reasonable that we may want it to be. Similarly, if we have other SPIs involved in the API, we'd want those to be configurable.
• DeclarativeConfigProperties will live in the API (i.e. as part of ConfigProvider for instrumentation config), and I think its a really good idea to add DeclarativeConfigProperties#getComponentLoader() so any component participating in config can use the same ComponentLoader / ClassLoader to load any required SPIs.

[jack-berg]

FYI new API surface area @jkwatson

[jkwatson]

It is a little weird to put this in the context package, but I do understand the reasons for it. It makes me wonder if we should just move the context into the API itself, rather than have it live separately. Is there a way we could do that in a non-breaking way, I wonder?

I won't hold up this PR, but it does end up feeling a little surprising to require someone to depend explicitly on the context artifact in order to load an SPI. 🤔

[jack-berg]

It makes me wonder if we should just move the context into the API itself, rather than have it live separately. Is there a way we could do that in a non-breaking way, I wonder?
it does end up feeling a little surprising to require someone to depend explicitly on the context artifact in order to load an SPI.

Remember that you don't have to explicitly depend on context since opentelemetry-api has a gradle API dependency on opentelemetry-context. Setting SPI loading aside, it would be really horrible if you needed to explicitly depend on opentelemetry-context to reference core things like Scope, Context, etc.

But yeah there's an argument that context and api should be combined. The decision to have them separate was before my time and if memory serves, the reasoning was that its useful for the java ecosystem to have a quality context propagation API that doesn't require you to bring other OpenTelemetry API concepts. I think this is a decent argument, but not sure if anyone is leveraging opentelemetry-context this way in practice. Also, not sure that someone looking for a general purpose context propgation API would come looking to the OpenTelemetry project, or reach the conclusion that it doesn't have any other dependencies and is a good candidate.

I think my main rub on this PR is the package this lives in: io.opentelemetry.context. I'd really like it to be in some generic utils package like io.opentelemetry.util, but I think with the module system its best practice for each artifact to have a single root package. And also, if we went with io.opentelemetry.util, the idea would be that we could put classes in that package from other modules like opentelemetry-api, and I think multiple artifacts contributing to a package is another no-no in the module system. Happy for someone to correct me on this.

@jkwatson will wait for your thoughts on this before proceeding because I want to get this right.

[jkwatson]

It makes me wonder if we should just move the context into the API itself, rather than have it live separately. Is there a way we could do that in a non-breaking way, I wonder?
But yeah there's an argument that context and api should be combined. The decision to have them separate was before my time and if memory serves, the reasoning was that its useful for the java ecosystem to have a quality context propagation API that doesn't require you to bring other OpenTelemetry API concepts. I think this is a decent argument, but not sure if anyone is leveraging opentelemetry-context this way in practice. Also, not sure that someone looking for a general purpose context propgation API would come looking to the OpenTelemetry project, or reach the conclusion that it doesn't have any other dependencies and is a good candidate.

I think my main rub on this PR is the package this lives in: io.opentelemetry.context. I'd really like it to be in some generic utils package like io.opentelemetry.util, but I think with the module system its best practice for each artifact to have a single root package. And also, if we went with io.opentelemetry.util, the idea would be that we could put classes in that package from other modules like opentelemetry-api, and I think multiple artifacts contributing to a package is another no-no in the module system. Happy for someone to correct me on this.

@jkwatson will wait for your thoughts on this before proceeding because I want to get this right.

I do like the idea of having a separate utils artifact for stuff like this. I would definitely invite a draft PR to see what that might look like.

[jack-berg]

I do like the idea of having a separate utils artifact for stuff like this. I would definitely invite a draft PR to see what that might look like.

I pushed a commit to do this. Couple of things to note / discuss:

• I originally wanted to use opentelemetry-api-common with root package io.opentelemetry.api.common. This would provide nice symmetry with opentelemetry-sdk-common. But it turns out opentelemetry-api already makes liberal use of this package so no go.
• Went with opentelemetry-api-util and io.opentelemetry.api.util, but could also be convinced to go with opentelemetry-common and io.opentelemetry.common instead. This alternative avoids the package collision problem, but makes it less clear this that is an "API common" artifact because "api" is removed from the artifact / package name.
• See io.opentelemetry.api.internal for other candidates to put in this API common artifact. Some of this code would be nice to leverage in opentelemetry-context but cannot because of the dependency graph. Finding a permanent home for shared internal code like this would contribute to Strengthen versioning requirements #6970.

[jkwatson]

Love it! Don't love YAA (yet another artifact), but structurally this feels correct to me. Thanks!

[jkwatson]

I do like the idea of having a separate utils artifact for stuff like this. I would definitely invite a draft PR to see what that might look like.

I pushed a commit to do this. Couple of things to note / discuss:

• I originally wanted to use opentelemetry-api-common with root package io.opentelemetry.api.common. This would provide nice symmetry with opentelemetry-sdk-common. But it turns out opentelemetry-api already makes liberal use of this package so no go.
• Went with opentelemetry-api-util and io.opentelemetry.api.util, but could also be convinced to go with opentelemetry-common and io.opentelemetry.common instead. This alternative avoids the package collision problem, but makes it less clear this that is an "API common" artifact because "api" is removed from the artifact / package name.
• See io.opentelemetry.api.internal for other candidates to put in this API common artifact. Some of this code would be nice to leverage in opentelemetry-context but cannot because of the dependency graph. Finding a permanent home for shared internal code like this would contribute to Strengthen versioning requirements #6970.

No strong opinion on io.opentelemetry.common vs. io.opentelemetry.api.util. Would be happy either way.

[jack-berg]

No strong opinion on io.opentelemetry.common vs. io.opentelemetry.api.util. Would be happy either way.

Went with io.opentelemetry.common and artifact name io.opentelemetry:opentelemetry-common. I prefer mirroring the naming of opentelemetry-sdk-common.