Copy ConfigProperties, ConfigurationException to opentelemetry-sdk-co… #5500
Conversation
| import javax.annotation.Nullable; | ||
|
|
||
| /** Properties used for configuration of the OpenTelemetry SDK components. */ | ||
| public interface ConfigProperties { |
There was a problem hiding this comment.
A small variation of this would be to make this version of ConfigProperties internal and not expose the EnvironmentResource#create(ConfigProperties) method. Would mean you can only access the environment resource using the environment variables and system properties obtains from System.* calls - no customizing by providing your own ConfigProperties implementation (except in the SPI).
There was a problem hiding this comment.
That might be a preferable starting point, then if we get requests for customization, we can consider exposing it? Still thinking through the implications of changing the parent interface on the one in the spi module. 🤔
There was a problem hiding this comment.
then if we get requests for customization, we can consider exposing it?
That was my thought.. but on second thought, it may not work. If we move the :sdk:common version to an internal package, then the version in :sdk-extensions:autoconfigure-spi will extend an internal interface. If we later wanted to change :sdk:common to be public facing, I'm pretty sure japicmp would complain.
|
Closing in favor #5554. |
In the 6/1 Java SIG we discussed an alternative to opentelemetry-java-instrumentation#8604 in which the environment resource logic and EnvironmentResourceProvider implementation of ResourceProvider SPI are moved
opentelemetry-sdk-common. The argument is that having this logic in the core of the SDK is more aligned with the spec language.The prerequisite of that is that we ConfigProperties, ConfigurationException to
opentelemetry-sdk-commonto avoid the circular dependency::sdk:common->:sdk-extensions:autoconfigure-spi->:sdk:commonThis PR shows what moving ConfigProperties, ConfigurationException looks like. Here's a summary:
:sdk:common:sdk-extensions:autoconfigure-spiversion of ConfigProperties to extend the:sdk:commonConfigProperties. Override all the methods, retaining the same contracts, to keep japicmp happy.:sdk-extensions:autoconfigure-spi. This implements the:sdk-extensions:autoconfigure-spiversion of ConfigurationProperties, and accepts the:sdk:commonas a delegate.It feels a bit strange to have two interfaces named ConfigProperties, and that the SPI interfaces accept the
:sdk-extensions:autoconfigure-spiversion to maintain backwards compatibility. But this should be a transparent change for users, as long as they're not using the internal DefaultConfigurationProperties class.WDYT? Is the payoff of having the environment resource more appropriately placed in
:sdk:commonenough to justify the awkwardness of having two identicalConfigurationPropertiesinterfaces?