[Spring starter] Make sdk bean overridable and cache env calls #15132
Conversation
github-actions
bot
added
the
test native
...lemetry/instrumentation/spring/autoconfigure/internal/properties/SpringConfigProperties.java
Outdated
Show resolved
Hide resolved
...lemetry/instrumentation/spring/autoconfigure/internal/properties/SpringConfigProperties.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| public String getString(String name) { | ||
| String normalizedName = ConfigUtil.normalizeEnvironmentVariableKey(name); | ||
| String value = environment.getProperty(normalizedName, String.class); |
There was a problem hiding this comment.
An alternative approach that could be considered would be to wrap the environment into a caching PropertyResolver. I presume that the sdk deliberately rereads these properties so they could be updated with opamp in the future. Idk whether this is something we should be concerned with.
There was a problem hiding this comment.
I presume that the sdk deliberately rereads these properties
no, it was not a deliberate decision as far as I know.
When I worked on the project, I assumed that properties would only be read at startup time.
I think it's hard to anticipate how OpAMP will be integrated, but we may simply flush the cache (from the caching PropertyResolver.
There was a problem hiding this comment.
implemented a caching property resolver, and included a way to flush the cache
| } | ||
|
|
||
| @Bean | ||
| @ConditionalOnMissingBean |
There was a problem hiding this comment.
i added this to allow people to customize the behavior, if they wanted
There was a problem hiding this comment.
we already have
- which I feel is enough, but feel free to create a separate PR to discussThere was a problem hiding this comment.
in the original issue #15009 they asked to be able to override this:
The auto-configuration should allow overriding or replacing SpringConfigProperties gracefully, for example by supporting @ConditionalOnMissingBean, so users can provide a cached or customized implementation without having to fork the library.
But since we addressed the performance issue, perhaps it's less necessary?
There was a problem hiding this comment.
yes, I think so
adding @ConditionalOnMissingBean increases the API surface area in ways that also increase complexity
...lemetry/instrumentation/spring/autoconfigure/internal/properties/SpringConfigProperties.java
Outdated
Show resolved
Hide resolved
3 participants
Fixes #15009
Add ability for users to customize AutoConfiguredOpenTelemetrySdk, and add cache for environment config lookups.