Skip to content
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 ability to programmatically customize configuration model when otel.experimental.config.file is set #6575

Open
jack-berg opened this issue Jul 11, 2024 · 7 comments
Labels
Feature Request Suggest an idea for this project

Comments

@jack-berg
Copy link
Member

The OTEL_EXPERIMENTAL_CONFIG_FILE spec describes a mechanism for the parsed configuration model to be customized:

Implementations MAY provide a mechanism to customize the configuration model parsed from OTEL_EXPERIMENTAL_CONFIG_FILE.

We should add support for this.

@harshitrjpt
Copy link

Hi @jack-berg ,
isn't AutoConfigurationCustomizerProvider already providing this capability of programmatically adding configurations?
Eg: https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/examples/extension/src/main/java/com/example/javaagent/DemoAutoConfigurationCustomizerProvider.java
Although, currently the SPI implementations are configured first before configuring from the file. But I suppose that order can be changed or not.
What are your thoughts ? Is my interpretation wrong ?

@jack-berg
Copy link
Member Author

AutoConfigurationCustomizerProvider does provide the capability for programmatically customizing configuration, but not for declarative configuration. We could make it so AutoConfigurationCustomizerProvider applies to declarative configuration as well, but that might not be the best idea:

  • Implementation probably expect AutoConfigurationCustomizerProvider to be called when env var / system property config is used. It probably doesn't make sense to always run AutoConfigurationCustomizerProvider when declarative config is used.
  • AutoConfigurationCustomizerProvider uses the ConfigProperties interface to represent the config. Implementations read from this to make decisions on what / how to customize. ConfigProperties is replaced by StructuredConfigProperties when declarative config is in use, which provides access to data in a significantly different way.

We might need to provide a declarative config alternative to AutoConfigurationCustomizerProvider.

Or we might say that declarative config customization is limited to customizing the config model (i.e. represented in code by OpenTelemetryConfiguration), with a new SPI something like:

public interface DeclarativeConfigurationCustomizer {
  OpenTelemetryConfiguration customizeModel(OpenTelemetryConfiguration);
}

@harshitrjpt
Copy link

harshitrjpt commented Sep 20, 2024

@jack-berg you are right, AutoConfigurationCustomizerProvider doesn't seem to be a good idea.

However, I investigated and explored more in direction of a new spi like DeclarativeConfigurationCustomizer. These were my observations/ findings (which in IMO achieves more genericness and abstraction)

  1. we can reuse the AutoConfigurationCustomizer interface and implement a new DeclarativeConfigurationSdkBuilder. Why? because AutoConfigurationCustomizer already provides the methods to achieve the functions of adding customizers for various components. But we'll need to make AutoConfigurationCustomizer more generic which gets extended by AutoConfiguredOpenTelemetrySdkBuilder and the new DeclarativeConfigurationSdkBuilder in following way.

  2. Like you mentioned the only issue in this case is that AutoConfigurationCustomizer uses ConfigProperties in the customizer methods.
    What I propose is that we can go with another super generic interface for properties like Properties extended by the ConfigProperties and StructuredConfigProperties, and use Properties in customizer functions. This will abstract the SDK implementation from the properties type- declarative or no-declarative.

  3. Although this will achieve the abstraction, but problem is that all the component configurations- ResourceConfiguration, MeterProviderConfiguration, etc are tightly coupled with ConfigProperties. So we'll need to modify the configurations to use more generic Properties. We'll need to assess how deep these changes would go.

  4. Coming back to 1, whatever we do,
    how do we abstract the SDK implementation from choosing which configuration- non-declarative or declarative, to be configured ? or
    we don't and let the SDK implementation consciously decide to use AutoConfiguredOpenTelemetrySdkBuilder or DeclarativeConfigurationSdkBuilder.

( 5. Also, I tried

public interface DeclarativeConfigurationCustomizer {
  OpenTelemetryConfiguration customizeModel(OpenTelemetryConfiguration);
}

in autoconfigure-spi. It will introduce circular dependency between modules 'opentelemetry-java.sdk-extensions.autoconfigure-spi.main' and 'opentelemetry-java.sdk-extensions.autoconfigure.main'.
Also IMO, this will introduce a very specific, different way to customize parse configs even though we already have (generic, if we make it) interface to customize parsed configs.
)

Thoughts, suggestions ?

@SylvainJuge
Copy link
Contributor

Hi,

Custom JMX metrics (JMX Insight) are currently supported through a stand-alone YAML file with otel.jmx.config option, having the ability to directly nest the configuration in the YAML configuration would be awesome as it removes the need to deal with an extra configuration file.

Do you have ideas how it could work for structured configuration like JMX ?

@trask
Copy link
Member

trask commented Dec 9, 2024

@SylvainJuge I think #6549 is what we need

@jack-berg
Copy link
Member Author

Do you have ideas how it could work for structured configuration like JMX ?

@SylvainJuge I think #6549 is what we need

Yes. The agent already has the ability to read existing flat properties out a structured declarative config YAML config file thanks to this.

But what we need for JMX metrics is:

  • Introduce ConfigProvider API #6549, so that jmx-metric instrumentation can get access to the relevant portion of the user's declarative config YAML
  • Update jmx-metric to look for this config, and if present, initialize accordingly

A simple jmx-metric config looks like:

rules:
  - bean: java.lang:type=Threading
    mapping:
      ThreadCount:
        metric: my.own.jvm.thread.count
        type: updowncounter
        desc: The current number of threads
        unit: "1"

When we take the steps outlined above, a user will be able to specify this in their declarative config YAML. For example, a user could set OTEL_EXPERIMENTAL_CONFIG_FILE=/path/to/my-config.yaml with contents:

resource: ... // omitted for brevity
tracer_provider: ... // omitted for brevity
meter_provider: ... // omitted for brevity
logger_provider: ... // omitted for brevity
instrumentation:
  java:
    jmx-metrics:
       rules:
          - bean: java.lang:type=Threading
            mapping:
              ThreadCount:
                metric: my.own.jvm.thread.count
                type: updowncounter
                desc: The current number of threads
                unit: "1"

jmx-metrics would:

  • Detect that declarative config is used by calling StrucutredConfigProperties instrumentationConfig = GlobalConfigProvider.get().getInstrumentationConfig() from Introduce ConfigProvider API #6549
  • Check if the user has configured jmx-metrics by calling StrucutredConfigProperties jmxMetricConfig = instrumentationConfig.getStructured("java").getStructured("jmx-metrics")
  • Translate StructuredConfigProperties jmxMetricConfig to the jmx-metrics internal representation and initialize accordingly

@SylvainJuge
Copy link
Contributor

This looks definitely nice !

From what I understand the top-level of the configuration file is for the SDK, and everything within .instrumentation relates to instrumentation, then we have a per-language breakdown.

For components that are part of "contrib" like the jmx-scraper that depend on instrumentation for their implementation, or other elements that only rely only on the SDK, it means their configuration would be nested in .instrumentation, which could be a bit confusing as no "instrumentation" is involved (here I'm biased by my usual Java perspective).

I haven't though much about this, but wouldn't it be simpler if we move the .instrumentation.java one level up and directly use .java ? Every language SDK will read exactly one item within the .instrumentation and ignore the others.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request Suggest an idea for this project
Projects
None yet
Development

No branches or pull requests

4 participants