-
Notifications
You must be signed in to change notification settings - Fork 848
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
add ComponentLoader to support more auto configuration scenarios #6217
Conversation
1c89890
to
1a6f001
Compare
@jack-berg please take a look 😄 |
b2539b2
to
703e039
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I can get behind this but need to think about it a little more. If we go in this direction, I think we'll want to incubate the ComponentLoader
class in an internal class for some period of time.
.../src/main/java/io/opentelemetry/sdk/autoconfigure/AutoConfiguredOpenTelemetrySdkBuilder.java
Outdated
Show resolved
Hide resolved
...toconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/MeterProviderConfiguration.java
Outdated
Show resolved
Hide resolved
ba945fc
to
5c9e395
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6217 +/- ##
============================================
- Coverage 91.06% 91.04% -0.02%
- Complexity 5695 5705 +10
============================================
Files 621 621
Lines 16667 16703 +36
Branches 1707 1709 +2
============================================
+ Hits 15177 15208 +31
- Misses 997 1002 +5
Partials 493 493 ☔ View full report in Codecov by Sentry. |
@jack-berg added all suggested changes - and even figured out how to appease jCmpApi (hopefully correctly). |
An additional SPI is needed for |
sorry, just saw that there is already |
1d8b60c
to
441ca55
Compare
...autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/internal/ComponentLoader.java
Show resolved
Hide resolved
...configure-spi/src/main/java/io/opentelemetry/sdk/autoconfigure/spi/ConfigurableProvider.java
Outdated
Show resolved
Hide resolved
.../src/main/java/io/opentelemetry/sdk/autoconfigure/AutoConfiguredOpenTelemetrySdkBuilder.java
Outdated
Show resolved
Hide resolved
.../src/main/java/io/opentelemetry/sdk/autoconfigure/AutoConfiguredOpenTelemetrySdkBuilder.java
Outdated
Show resolved
Hide resolved
...oconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/TracerProviderConfiguration.java
Outdated
Show resolved
Hide resolved
441ca55
to
d03a67f
Compare
@@ -368,6 +369,13 @@ public AutoConfiguredOpenTelemetrySdkBuilder setServiceClassLoader( | |||
return this; | |||
} | |||
|
|||
/** Sets the {@link ComponentLoader} to be used to load SPI implementations. */ | |||
public AutoConfiguredOpenTelemetrySdkBuilder setComponentLoader(ComponentLoader componentLoader) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this is problematic. Can't have a public API reference an internal class, since this would break when we move ComponentLoader to a public package. The way we've implemented this stuff in the past is:
- ComponentLoader is in an internal package
AutoConfiguredOpenTelemetrySdkBuilder#setComponentLoader
is package private- A public static method is added to a class in an internal package which reflectively calls
AutoConfiguredOpenTelemetrySdkBuilder#setComponentLoader
. The natural place for this is AutoConfigureUtil. If a user wants to use the experimental functionality, they can call the public static method on the internalAutoConfigureUtil
until we promote the functionality to the public API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, that's why we have those reflective methods 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its strange but it works 😅
@@ -115,7 +116,7 @@ public final class AutoConfiguredOpenTelemetrySdkBuilder implements AutoConfigur | |||
* {@link #addPropertiesSupplier(Supplier)} and {@link #addPropertiesCustomizer(Function)} will | |||
* have no effect if this method is used. | |||
*/ | |||
AutoConfiguredOpenTelemetrySdkBuilder setConfig(ConfigProperties config) { | |||
public AutoConfiguredOpenTelemetrySdkBuilder setConfig(ConfigProperties config) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain the reasoning for this? Its pretty drastic to want to call this and negate calls to addPropertiesSupplier
and addPropertiesCustomizer
.
Its currently only being used in tests, and even those usages aren't necessary. I honestly think we ought to delete it altogether rather than promote it to the public API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spring needs a different implementation of ConfigProperties
, because of limitations in list and map handling - see https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/d8aa0f5b48e2dee28498b6f95d3d0017eb673cbf/instrumentation/spring/spring-boot-autoconfigure/src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/resources/SpringConfigProperties.java#L102-L117
The alternative would be to stringify maps and lists - and then let them be parsed using DefaultConfigProperties
- that doesn't look right, IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestion: Make this method experimental like setComponentLoader
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestion: Make this method experimental like setComponentLoader
What would be the path to stabilizing? I think we should stabilize setComponentLoader
after we integrate it into spring autoconfigure and confirm there we didn't miss anything. But overriding the config here conflicts with autoconfigure patterns by erasing property suppliers and customizers provided via SPI. Overriding ConfigProperties means that spring autoconfigure would interact with most SPIs normally, except those involving customizing config properties. Would be surprising to users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about an experimental method to customize ConfigProperties? Something like:
AutoConfiguredOpenTelemetrySdkBuilder setConfigPropertiesCustomizer(Function<ConfigProperties, ConfigProperties> configPropertiesCustomizer)
The idea is that it follows a similar pattern to the customization methods in AutoConfigurationCustomizer
like AutoConfigurationCustomizer#addPropertiesCustomizer(Function<ConfigProperties, Map<String, String>>)
, but is able to return a custom ConfigProperties
instance instead of Map<String, String>
. We can limit this method to AutoConfigurationOpenTelemetrySdkBuilder
(i.e. do not add it to AutoConfigurationCustomizer
). Spring autoconfiguration's implementation of ConfigProperties
can fallback to the ConfigProperties
instance provided by autoconfigure if a particular property is undefined in spring's configuration system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That idea is quite elegant.
I don't think a spring user would expect that capability, because spring can already do that: https://stackoverflow.com/questions/29072628/how-can-i-override-spring-boot-application-properties-programmatically
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think your proposal could be good for distros of the spring starter, so I'll implement it.
@@ -115,6 +115,8 @@ if (!project.hasProperty("otel.release") && !project.name.startsWith("bom")) { | |||
// Reproduce defaults from https://github.com/melix/japicmp-gradle-plugin/blob/09f52739ef1fccda6b4310cf3f4b19dc97377024/src/main/java/me/champeau/gradle/japicmp/report/ViolationsGenerator.java#L130 | |||
// with some changes. | |||
val exclusions = mutableListOf<String>() | |||
// Generics are not detected correctly | |||
exclusions.add("CLASS_GENERIC_TEMPLATE_CHANGED") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't need this anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not for this PR - but it's still true that jApiCmp will trip over generics - I'm fine to take it out, of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple more comments but I think we're close.
{ | ||
double ratio = | ||
config.getDouble("otel.traces.sampler.arg", DEFAULT_TRACEIDRATIO_SAMPLE_RATIO); | ||
return Sampler.parentBased(Sampler.traceIdRatioBased(ratio)); | ||
} | ||
return Sampler.parentBased(ratioSampler(config)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a nice change, but seems unrelated to this PR? in the future splitting out unrelated changes can make life easier for reviewers
/** | ||
* Load implementations of an ordered SPI (i.e. implements {@link Ordered}). | ||
* | ||
* @param spiClass the SPI class | ||
* @param <T> the SPI type | ||
* @return list of SPI implementations, in order | ||
*/ | ||
default <T extends Ordered> List<T> loadOrdered(Class<T> spiClass) { | ||
return StreamSupport.stream(load(spiClass).spliterator(), false) | ||
.sorted(Comparator.comparing(Ordered::order)) | ||
.collect(Collectors.toList()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this method? (can inline as private method in SpiHelper
?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the method is meant for overriding.
In spring, the spring beans are given a higher priority - https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/10453/files#diff-8c1885af89564bfbd461edbbf02fe19e23e40a2e54f6e264d41d786b760e5d26R40-R53.
I'm still unsure if that's what users actually expect though.
It affects
- AutoConfigurationCustomizerProvider
- ResourceProvider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't find a really compelling reason - I'll remove it.
@jack-berg can you check again? |
Thanks @jack-berg for getting it into this release 🎉 |
Part of open-telemetry/opentelemetry-java-instrumentation#10409
Here's a PoC to show that the functionality actually works for spring boot: open-telemetry/opentelemetry-java-instrumentation#10453
Why do we need the new component loader public interface?
Let's say a spring user wants to create a new span exporter - then they would create the following spring class:
(They would also need to set
otel.spans.exporter
to use the span exporter.)Normally this would not have any effect on the SDK auto-config, because Spring beans are different from the SPI way of loading things.
The component loader allows the spring starter to create an adapter, which is shown here