Define dedicated file configuration SPI ComponentProvider by jack-berg · Pull Request #6457 · open-telemetry/opentelemetry-java

jack-berg · 2024-05-14T16:23:43Z

A while ago I opened PR #5912, which introduced ExtendedConfigProperties. The idea was to have an extended version of the ConfigProperties interface which included new accessors for reading structured data in addition to the simple types already exposed by ConfigProperties. Then, when file configuration needed to load a custom SDK extension component like SpanExporter, it would construct a ExtendedConfigProperties from the YAML contents and pass it to the SPI (i.e. ConfigurableSpanExporterProvider.

This approach is problematic because it requires SPI implementations to understand whether they're working on a file configuration context or not based on whether the ConfigProperties is actually an instance of ExtendedConfigProperties. The shape of data and the access patterns is quite different for file configuration vs env var / system property based configuration, so this whole thing is error prone and rather confusing.

In this PR, I propose a different direction which embraces the spec decision that file configuration should be thought of as the v2 of configuration, rather than something that is an extension of the existing env var / system property mechanism. Here's how I'm thinking about it:

File configuration is a distinct configuration path from env var / system property configuration, including completely separate SPIs.
- If you set otel.experimental.config.file you're opting into this separate path. If you don't, all the existing SPIs and config mechanism works.
- This separation is intention. It should be less confusing than the alternative where SPIs and ConfigProperties serve in multiple capacities.
We introduce a generic ComponentProvider SPI, reflecting the spec definition.
- ComponentProvider is a single interface to provide instances of all the SDK extension points (Sampler, SpanExporter, MetricExporter, Propagator, etc).
- Users implement this SPI to provide custom named SDK extension components in a file configuration context. It is never called outside a file configuration context.
We introduce a new StructuredConfigProperties for reading structured config data.
- This interface is similar to ConfigProperties, but does not inherit from it.
- It has new methods for reading structured data. Its interface for reading primitives is slightly modified to reflect the required semantics of representing a YAML node (i.e. getScalarList is nullable so callers can differentiate between not set and set but empty).
- This interface is passed to ComponentProvider#create(StructuredConfigProperties).
Component authors who wish for their components to be accessible in both an env var / system property and an structured file config context must implement both SPIs. In doing so, they are made very aware of the differences between the two and difference in config schema.

For demonstration purposes and to make this PR as simple as possible, I've implemented a single ComponentProvider in the opentelemetry-exporters-prometheus artifact. If this PR is merged, I will followup by implementing ComponentProvider for the other built in components.

…uration

…y-java into extended-config-properties

…y-java into extended-config-properties-1

jack-berg · 2024-05-14T16:24:36Z

...src/main/java/io/opentelemetry/exporter/prometheus/internal/PrometheusComponentProvider.java

+ * <p>This class is internal and is hence not for public use. Its APIs are unstable and can change
+ * at any time.
+ */
+public class PrometheusComponentProvider implements ComponentProvider<MetricReader> {


A demonstration ComponentProvider. Notice how config properties are accessed without dot notation since the YAML node is presented directly as the StructuredConfigProperties.

jack-berg · 2024-05-14T16:25:11Z

...rc/main/java/io/opentelemetry/sdk/autoconfigure/spi/internal/StructuredConfigProperties.java

+   * @throws ConfigurationException if the property is not a sequence of mappings
+   */
+  @Nullable
+  List<StructuredConfigProperties> getStructuredList(String name);


getStructured and getStructuredList are the key bits which allow reading complex data.

jack-berg · 2024-05-14T16:26:37Z

...nfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/AutoConfiguredOpenTelemetrySdk.java

+   * @see #getConfig()
+   */
+  @Nullable
+  abstract StructuredConfigProperties getStructuredConfig();


This new method allows callers to determine if autoconfiguration was driven by file configuration or by env vars / system properties. Either this or getConfig will return null. The other will be non-null.

If file configuration is used, getStructuredConfig represents the full YAML config file.

This is kinda the only unfortunate downside of this approach that I can find thus far. It's a small drag that users of the autoconfigured sdk now have to be aware or otherwise check which config was used to get the data they need.

It feels like there is room to flatten the structured config into ConfigProperties and also to objectify the ConfigProperties into a structured thing...but we can have that conversation another time.

It's a small drag that users of the autoconfigured sdk now have to be aware or otherwise check which config was used to get the data they need.

I would characterize it slightly differently: Providers of custom components like exporters and samplers need to be aware that somebody can use file based config or flat system property / env var based config, and provide different SPI implementations for each. This is frustrating, but seems to be the inevitable result of otel getting as far as it did without a proper configuration story. As I mentioned in the PR description, I started with the opposite approach and tried to have a single unified ConfigProperties for both configuration routes. I've concluded while this is simpler from an API standpoint, its more confusing and brittle for users, since even though we can do some gymnastics to make ConfigProperties work for both cases, the data presented to SPI implementers will be different (i.e. the property names and structure and semantics will be different for flat env vars than in structured config). So there appears to be no getting around confronting the user with this difference. Might as well make it as explicit and obvious as possible.

jack-berg · 2024-05-14T16:28:58Z

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

+    assertThat(otherProps.getString("str_key")).isEqualTo("str_value");
+    StructuredConfigProperties otherMapKeyProps = otherProps.getStructured("map_key");
+    assertThat(otherMapKeyProps).isNotNull();
+    assertThat(otherMapKeyProps.getString("str_key1")).isEqualTo("str_value1");


This test demonstrates how a consumers like the otel java agent could access complex config data which is not part of the core file configuration schema. The other key and the structure it encloses is not part of the core schema yet its contents are accessible.

This will be important for configuring java agent specific options, like content for the JMX integration.

cc @trask, @laurit

jack-berg · 2024-05-14T16:30:31Z

...r/src/main/java/io/opentelemetry/sdk/extension/incubator/fileconfig/MetricReaderFactory.java

+        MetricReader metricReader =
+            FileConfigUtil.loadComponent(
+                spiHelper, MetricReader.class, "prometheus", prometheusModel);
+        return FileConfigUtil.addAndReturn(closeables, metricReader);


Here's what it looks like to load a component in the file configuration implementation. We state that we want ComponentProvider which provides instances of MetricReader.class named prometheus, and we pass the prometheusModel to it (which is translated to StructuredConfigProperties).

Nice and easy.

codecov · 2024-05-14T16:36:42Z

Codecov Report

Attention: Patch coverage is 73.86364% with 46 lines in your changes are missing coverage. Please review.

Project coverage is 90.72%. Comparing base (f579061) to head (c6af54c).
Report is 1 commits behind head on main.

Files	Patch %	Lines
...tor/fileconfig/YamlStructuredConfigProperties.java	75.45%	17 Missing and 10 partials ⚠️
...extension/incubator/fileconfig/FileConfigUtil.java	43.47%	10 Missing and 3 partials ⚠️
.../sdk/autoconfigure/internal/AutoConfigureUtil.java	0.00%	5 Missing ⚠️
...ometheus/internal/PrometheusComponentProvider.java	90.90%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #6457      +/-   ##
============================================
- Coverage     90.86%   90.72%   -0.14%     
- Complexity     6155     6214      +59     
============================================
  Files           675      678       +3     
  Lines         18461    18621     +160     
  Branches       1813     1828      +15     
============================================
+ Hits          16774    16894     +120     
- Misses         1151     1180      +29     
- Partials        536      547      +11

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

breedx-splk

@jack-berg this is a fantastic step forward on the config front!

...ure-spi/src/main/java/io/opentelemetry/sdk/autoconfigure/spi/internal/ComponentProvider.java

...rc/main/java/io/opentelemetry/sdk/autoconfigure/spi/internal/StructuredConfigProperties.java

...java/io/opentelemetry/sdk/extension/incubator/fileconfig/YamlStructuredConfigProperties.java

breedx-splk · 2024-05-17T21:52:14Z

...nfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/AutoConfiguredOpenTelemetrySdk.java

+   * @see #getConfig()
+   */
+  @Nullable
+  abstract StructuredConfigProperties getStructuredConfig();


This is kinda the only unfortunate downside of this approach that I can find thus far. It's a small drag that users of the autoconfigured sdk now have to be aware or otherwise check which config was used to get the data they need.

It feels like there is room to flatten the structured config into ConfigProperties and also to objectify the ConfigProperties into a structured thing...but we can have that conversation another time.

...ubator/src/main/java/io/opentelemetry/sdk/extension/incubator/fileconfig/FileConfigUtil.java

jack-berg · 2024-05-24T14:36:29Z

@breedx-splk I would greatly appreciate another look when you have a chance 🙂

breedx-splk

This looks like a good way to proceed. I wouldn't mind having another set of eyes from @trask or @laurit on the instrumentation side as well, but seems good from where I stand.

jack-berg · 2024-05-30T14:31:00Z

@open-telemetry/java-approvers will merge tomorrow if there are no additional comments.

…y-java into component-provider

...ure-spi/src/main/java/io/opentelemetry/sdk/autoconfigure/spi/internal/ComponentProvider.java

...rc/main/java/io/opentelemetry/sdk/autoconfigure/spi/internal/StructuredConfigProperties.java

…der SPI

jack-berg added 6 commits October 12, 2023 14:33

Add ExtendedConfigProperties to access complex types from file config…

af278ba

…uration

Add getConfigProperties implementation to DefaultConfigProperties

9d00503

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

163dcd4

…y-java into extended-config-properties

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

fccde9c

…y-java into extended-config-properties-1

Decouple StructuredConfigProperties from ConfigProperties

2ada3e5

Introduce component provider SPI, implement for reader

f9c28e9

jack-berg requested a review from a team May 14, 2024 16:23

jack-berg commented May 14, 2024

View reviewed changes

jack-berg mentioned this pull request May 14, 2024

Extended config properties #5912

Closed

breedx-splk reviewed May 17, 2024

View reviewed changes

PR feedback

ab07587

breedx-splk approved these changes May 29, 2024

View reviewed changes

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

8ea3bdd

…y-java into component-provider

trask reviewed May 30, 2024

View reviewed changes

...ure-spi/src/main/java/io/opentelemetry/sdk/autoconfigure/spi/internal/ComponentProvider.java Show resolved Hide resolved

...rc/main/java/io/opentelemetry/sdk/autoconfigure/spi/internal/StructuredConfigProperties.java Outdated Show resolved Hide resolved

jack-berg added 2 commits May 31, 2024 17:31

Add type safe scalar accessor, accessor for all property keys

2ed3468

Add todo to consider dynamic config before stabilizing ComponentProvi…

c6af54c

…der SPI

jack-berg merged commit d9cef81 into open-telemetry:main Jun 3, 2024

This was referenced Jun 3, 2024

Add file configuration ComponentProvider support for exporters #6493

Merged

Add file configuration ComponentProvider support for samplers #6494

Merged

jack-berg mentioned this pull request Jul 11, 2024

Add file configuration ComponentProvider support for all SDK extension points #6574

Closed

6 tasks

Conversation

jack-berg commented May 14, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented May 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

breedx-splk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jack-berg commented May 24, 2024

Uh oh!

breedx-splk left a comment

Choose a reason for hiding this comment

Uh oh!

jack-berg commented May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented May 14, 2024 •

edited

Loading

jack-berg commented May 30, 2024 •

edited

Loading