Introduce ConfigProvider API #6549

jack-berg · 2024-07-02T20:44:45Z

This PR introduces a new category of API surface area called ConfigProvider, which is analog to TracerProvider, MeterProvider, LoggerProvider.

ConfigProvider (and its corresponding global accessor GlobalConfigProvider) provides a mechanism for instrumentation to read data from the new .instrumentation portion of declarative configuration.

Why does instrumentation need to participate in config?

To provide a good user experience, we should centralize all otel config a user might provide in a single place. Limiting declarative config to SDK configuration just kicks the problem of instrumentation config down the road when the problem domains are quite similar.

Why do we need config surface area in the API instead of just SDK?

Historically, we've limited configuration to the SDK and the related autoconfigure module. (Although long ago we debated whether certain classes from the opentelemetry-extension-autoconfigure-spi like ConfigProperties should be in the opentelemetry-api instead). This pattern breaks down with instrumentation config because we want native instrumentation to read from declarative config without taking a dependency on any SDK artifacts.

What's the user flow for interacting with ConfigProvider?

Let's use the otel java agent as an example. Supposing a user opts into declarartive configuration by setting OTEL_EXPERIMENTAL_CONFIG_FILE=/path/to/config.yaml, the agent will install an SDK configured according to the file contents using logic in opentelemetry-sdk-extension-autoconfigure / opentelemetry-sdk-extension-incubator, which are bundled with the otel java agent.

This PR extends the autoconfigure logic to initialize a ConfigProvider instance when OTEL_EXPERIMENTAL_CONFIG_FILE is specified, which is accessible in via:

AutoConfigureUtil#getConfigProvider()
GlobalConfigProvider#get()

ConfigProvider has just a single operation right now:

  @Nullable
  StructuredConfigProperties getInstrumentationConfig();

Where StructuredConfigProperties represents the YAML node at .instrumentation from the user's config file. I've added convenience methods in InstrumentationConfigUtil to read common properties:

public static Map<String, String> peerServiceMapping(ConfigProvider configProvider) {}
public static List<String> httpClientRequestCapturedHeaders(ConfigProvider configProvider) {}
public static List<String> httpClientResponseCapturedHeaders(ConfigProvider configProvider) {}
public static List<String> httpServerRequestCapturedHeaders(ConfigProvider configProvider) {}
public static List<String> httpSeverResponseCapturedHeaders(ConfigProvider configProvider) {}

And a convenience method for accessing properties specific to a particular instrumentation library:

public static StructuredConfigProperties javaInstrumentationConfig(ConfigProvider configProvider, String instrumentationName)

The agent would need to be updated to detect that file configuration was used, and to configure the various instrumentation libraries it installs according to the installed ConfigProvider.

Any native instrumentation can similarly read ConfigProvider on initialization and configure itself accordingly.

What does this PR do to make all this work?

There are lot of files changed to accomplish this, but they mostly shift existing code around to move the required libraries to opentelemetry-api-incubator module. Notably, the spec PR introducing the instrumentation configuration API rebrands "file configuration" to be "declarative configuration". Its best to do user facing class renames at the same time that those classes are moving packages to minimize churn.

Here's a summary of the changes:

Introduce new io.opentelemetry.api.incubator.config package in opentelemetry-api-incubator to hold all classes / interfaces required for the config API
Introduce new ConfigProvider, GlobalConfigProvider, InstrumentationConfigUtil representing the config API
Move StructuredConfigProperties from opentelemetry-sdk-extension-incubator to opentelemetry-api-incubator, rename to DeclarativeConfigProperties
Replace all file config usages of ConfigurationException with new DeclarativeConfigurationException which lives in opentelemetry-api-incubator and can be referenced in API

brunobat · 2024-07-18T16:58:39Z

From what I understand, if you provide an implementation for StructuredConfigProperties, then the getConfig():

opentelemetry-java/sdk-extensions/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/AutoConfiguredOpenTelemetrySdkBuilder.java

Line 429 in d9cef81

ConfigProperties config = getConfig();

Will return your implementation, If you provide no implementation, then the existing propertiesSupplier and propertiesCustomizerswill kick in.
Right?

jack-berg · 2024-07-19T22:10:05Z

Will return your implementation, If you provide no implementation, then the existing propertiesSupplier and propertiesCustomizerswill kick in. Right?

No. This API only provides StructuredConfigProperties, and alternative to ConfigProperties which is akin to a YAML mapping node. Its possible for ConfigProvider to return something useful when file config is not used, but we'd need to design that mechanism.

brunobat · 2024-07-22T09:35:34Z

No. This API only provides StructuredConfigProperties, and alternative to ConfigProperties which is akin to a YAML mapping node. Its possible for ConfigProvider to return something useful when file config is not used, but we'd need to design that mechanism.

Sorry, I'm confused.
From this code:

 private ConfigProperties getConfig() {
    ConfigProperties config = this.config;
    if (config == null) {
      config = computeConfigProperties();
    }
    return config;
  }

If no structured properties are present, properties will be computed from current propertiesSupplier and propertiesCustomizers... If this is not the case, then it's a breaking change...

jack-berg · 2024-07-22T14:08:29Z

If no structured properties are present, properties will be computed from current propertiesSupplier and propertiesCustomizers... If this is not the case, then it's a breaking change...

Sorry I must have misunderstood your question. This PR introduces net new (experimental) API surface area in the opentelemetry-api-incubator module. None of the APIs in the stable opentelemetry-sdk-extension-autoconfigure module are changed / impacted.

brunobat · 2024-07-22T14:23:43Z

thanks

codefromthecrypt

handy. hope the review helps despite being not authoritative @jack-berg!

api/incubator/src/main/java/io/opentelemetry/api/incubator/config/ConfigProvider.java

api/incubator/src/main/java/io/opentelemetry/api/incubator/config/GlobalConfigProvider.java

...incubator/src/main/java/io/opentelemetry/api/incubator/config/InstrumentationConfigUtil.java

codefromthecrypt · 2024-07-26T05:08:15Z

...bator/src/test/java/io/opentelemetry/api/incubator/config/InstrumentationConfigUtilTest.java

+    assertThat(InstrumentationConfigUtil.peerServiceMapping(emptyHttpConfigProvider)).isNull();
+  }
+
+  @Test


recommend tests for below cases possibly something that can be refactored to a common fixture for all, but even if explicit 1. path is incomplete 2. middle of the path is the wrong type 3. end of the path is the wrong type 4. end of the path is empty

sdk-extensions/autoconfigure-spi/build.gradle.kts

.../src/main/java/io/opentelemetry/sdk/autoconfigure/AutoConfiguredOpenTelemetrySdkBuilder.java

codefromthecrypt · 2024-07-26T05:10:41Z

...figure/src/testFullConfig/java/io/opentelemetry/sdk/autoconfigure/FileConfigurationTest.java

-    StructuredConfigProperties otherMapKeyProps = otherProps.getStructured("map_key");
-    assertThat(otherMapKeyProps).isNotNull();
-    assertThat(otherMapKeyProps.getString("str_key1")).isEqualTo("str_value1");
+    ConfigProvider globalConfigProvider = GlobalConfigProvider.get();


if we care, we run this again to test that the value returned is a constant

...or/src/main/java/io/opentelemetry/sdk/extension/incubator/fileconfig/FileConfigProvider.java

…y-java into config-provider

jack-berg · 2024-07-31T23:11:05Z

handy. hope the review helps despite being not authoritative @jack-berg!

Thanks for your time - lots of good advice in here 🙂

codecov · 2024-07-31T23:26:30Z

Codecov Report

Attention: Patch coverage is 77.30496% with 32 lines in your changes missing coverage. Please review.

Project coverage is 90.04%. Comparing base (a6a9acb) to head (81f601c).

Files with missing lines	Patch %	Lines
...pi/incubator/config/InstrumentationConfigUtil.java	86.66%	1 Missing and 3 partials ⚠️
...incubator/fileconfig/DeclarativeConfiguration.java	71.42%	2 Missing and 2 partials ⚠️
...or/fileconfig/YamlDeclarativeConfigProperties.java	77.77%	4 Missing ⚠️
...elemetry/sdk/autoconfigure/internal/SpiHelper.java	33.33%	2 Missing ⚠️
...nsion/incubator/fileconfig/AggregationFactory.java	0.00%	2 Missing ⚠️
...incubator/fileconfig/LogRecordExporterFactory.java	0.00%	2 Missing ⚠️
...ncubator/fileconfig/LogRecordProcessorFactory.java	0.00%	2 Missing ⚠️
...on/incubator/fileconfig/MetricExporterFactory.java	0.00%	2 Missing ⚠️
...extension/incubator/fileconfig/SamplerFactory.java	0.00%	2 Missing ⚠️
...sion/incubator/fileconfig/SpanExporterFactory.java	0.00%	2 Missing ⚠️
... and 5 more

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #6549      +/-   ##
============================================
+ Coverage     90.02%   90.04%   +0.01%     
- Complexity     6488     6516      +28     
============================================
  Files           719      724       +5     
  Lines         19587    19643      +56     
  Branches       1931     1938       +7     
============================================
+ Hits          17634    17687      +53     
  Misses         1354     1354              
- Partials        599      602       +3

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

Resolves #3535. This introduces an API component to file configuration, which has been limited to SDK (i.e. end user facing) up until this point. The configuration model recently added the first surface area related to instrumentation configuration properties in open-telemetry/opentelemetry-configuration#91. The API proposed in this PR is collectively called the "Instrumentation config API", and provides a mechanism for instrumentation libraries to participate in file configuration and read relevant properties during initialization. The intent is for both OpenTelemetry-authored and native instrumentation alike to be able to be configured by users in a standard way. New API surface area is necessary to accomplish this to avoid instrumentation libraries from needing to take a dependency on SDK artifacts. The following summarizes the additions: - Introduce ConfigProvider, the instrumentation config API analog of TracerProvider, MeterProvider, LoggerProvider. This is the entry point to the API. - Define "Get instrumentation config" operation for ConfigProvider. This returns something called ConfigProperties, which is a programmatic representation of a YAML mapping node. The ConfigProperties returned by "Get instrumentation config" represents the [`.instrumentation`](https://github.com/open-telemetry/opentelemetry-configuration/blob/670901762dd5cce1eecee423b8660e69f71ef4be/examples/kitchen-sink.yaml#L438-L439) node defined in a config file. - Rebrand "file configuration" to "declarative configuration". This expresses the intent without coupling to the file representation, which although will be the most popular way to consume these features is just one possible way to represent the configuration model and use these tools. - Break out dedicated `api.md`, `data-model.md`, and `sdk.md` files for respective API, data model, and SDK portions of declarative configuration. This aligns with other portions of the spec. The separation should improve clarity regarding what should and should not be exposed in the API. I've prototyped this new API in `opentelemetry-java` here: open-telemetry/opentelemetry-java#6549 cc @open-telemetry/configuration-maintainers, @open-telemetry/specs-semconv-maintainers

…y-java into config-provider

jack-berg · 2024-08-21T20:11:26Z

The spec PR introducing the configuration API is merged and I'm now seeking reviews for this with the intent to merge.

codefromthecrypt

@geoand @ThomasVitale I know both of you work in different frameworks, so that's a good thing ;) Imagine in LLM a setting for disabling prompt logging, and this would be obtained by a config provider, using config semantics that also apply to other languages like python or Go.

In understand that in your frameworks, config loading compat might not be a specific goal, but if you have any commentary around this impl, it would be much obliged. conventional and standardized loading is very helpful when agents are in use.

geoand · 2024-08-23T05:13:48Z

Thanks for the ping!

I'll have a look next week when I'm back from PTO

…y-java into config-provider

Introduce ConfigProvider API

c5cdc1c

jack-berg requested a review from a team July 2, 2024 20:44

jack-berg mentioned this pull request Jul 3, 2024

Define instrumentation configuration API open-telemetry/opentelemetry-specification#4128

Merged

codefromthecrypt approved these changes Jul 26, 2024

View reviewed changes

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

6ec7a0a

…y-java into config-provider

partially incorporate pr feedback

a624c70

jack-berg added 2 commits August 20, 2024 16:42

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

e986018

…y-java into config-provider

Rename to declarative configuration

0389e1c

codefromthecrypt reviewed Aug 22, 2024

View reviewed changes

jack-berg mentioned this pull request Aug 29, 2024

Declarative configuration missing pieces #6677

Merged

jack-berg mentioned this pull request Sep 23, 2024

Support declarative configuration open-telemetry/opentelemetry-java-instrumentation#12265

Merged

jack-berg added 2 commits October 3, 2024 14:00

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

64df541

…y-java into config-provider

Simplify InstrumentationConfigUtil

b184a59

jack-berg requested a review from a team as a code owner October 3, 2024 19:15

Cleanup, reduce dependencies to compileOnly

81f601c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce ConfigProvider API #6549

Introduce ConfigProvider API #6549

jack-berg commented Jul 2, 2024 •

edited

Loading

brunobat commented Jul 18, 2024

jack-berg commented Jul 19, 2024

brunobat commented Jul 22, 2024

jack-berg commented Jul 22, 2024

brunobat commented Jul 22, 2024

codefromthecrypt left a comment

codefromthecrypt Jul 26, 2024

codefromthecrypt Jul 26, 2024

jack-berg commented Jul 31, 2024

codecov bot commented Jul 31, 2024 •

edited

Loading

jack-berg commented Aug 21, 2024

codefromthecrypt left a comment

geoand commented Aug 23, 2024

Introduce ConfigProvider API #6549

Are you sure you want to change the base?

Introduce ConfigProvider API #6549

Conversation

jack-berg commented Jul 2, 2024 • edited Loading

brunobat commented Jul 18, 2024

jack-berg commented Jul 19, 2024

brunobat commented Jul 22, 2024

jack-berg commented Jul 22, 2024

brunobat commented Jul 22, 2024

codefromthecrypt left a comment

Choose a reason for hiding this comment

codefromthecrypt Jul 26, 2024

Choose a reason for hiding this comment

codefromthecrypt Jul 26, 2024

Choose a reason for hiding this comment

jack-berg commented Jul 31, 2024

codecov bot commented Jul 31, 2024 • edited Loading

Codecov Report

jack-berg commented Aug 21, 2024

codefromthecrypt left a comment

Choose a reason for hiding this comment

geoand commented Aug 23, 2024

jack-berg commented Jul 2, 2024 •

edited

Loading

codecov bot commented Jul 31, 2024 •

edited

Loading