Support declarative configuration #12265

jack-berg · 2024-09-16T20:49:13Z

When declarative config is used, AutoConfigureUtil.getConfig(AutoConfiguredOpenTelemetrySdk) returns null.

This adjusts all calls to this method to be able to accommodate the null response.

Introduces StructuredConfigPropertiesBridge, which implements ConfigProperties when structured config is used and reads agent / instrumentation configuration from the YAML source. See comment for more details.

jack-berg · 2024-09-20T21:04:31Z

...java/io/opentelemetry/javaagent/bootstrap/internal/StructuredConfigPropertiesBridgeTest.java

+import java.util.Map;
+import org.junit.jupiter.api.Test;
+
+class StructuredConfigPropertiesBridgeTest {


StructuredConfigPropertiesBridge is my proposal for how we keep a sane configuration story in the agent / instrumentation despite the SDK having either flat sys prop / env var or structured YAML configuration options.

The idea is the implement the ConfigProperties interface using a backing StructuredConfigProperties instance, and have special logic to determine how the [standard java agent](https://opentelemetry.io/docs/zero-code/java/agent/disable/ properties map to a structured representation.

Only properties starting with otel.instrumentation. resolve

otel.instrumentation. refers to the node at .instrumentation.java in the declarative config data model

To resolve a property, we take the portion after otel.instrumentation and break it up into segments. For each segment (N-1) we walk down the StructuredConfigProperties tree, and read the final segment from the resolve node.

So given the following YAML, reading configProperties.getBoolean("otel.instrumentation.common.default-enabled") resolves true:

instrumentation: java: common: default-enabled: true

See open-telemetry/opentelemetry-java-examples#492 for a demonstration

(Note, requires building the javaagent locally, including this PR from opentelemetry-java)

long-term, would we bridge in the other direction? i.e. replace ConfigProperties usage with StructuredConfigProperties everywhere in the Java agent, and have a bridge from ConfigProperties -> StructuredConfigProperties?

That's certainly an option. I think we ought to talk through options and repercussions as a group.

One thing to note is that the access patterns for StructuredConfigProperties and ConfigProperties are quite different.

For example, accessing otel.instrumentation.common.default-enabled is something like:

ConfigProperties: config.getBoolean("otel.instrumentation.common.default-enabled")

StructuredConfigProperties (assuming you're starting at the .instrumentation.java node) config.getStructured("config").getBoolean("default-enabled")

needs additional null handling each time you walk down a node

diverges further for each level of nesting

Also need to remember that there's now a specification for an instrumentation config API (and java PR here open-telemetry/opentelemetry-java#6549). The java agent works around the historic lack of availability of such a thing, and we'll need to think about how / when / (if?) to cut over. There's lots of instrumentation that reads config - is it feasible for a one time cutover to the new APIs, or do we need to plan for some sort of iterative approach?

needs additional null handling each time you walk down a node

what do you think of optional return types that support subsequent accessors without null check?

diverges further for each level of nesting

I didn't follow what you mean?

we'll need to think about how / when / (if?) to cut over. There's lots of instrumentation that reads config - is it feasible for a one time cutover to the new APIs, or do we need to plan for some sort of iterative approach?

if we do some initial prototyping and like it, we could do a one-time cutover in the 3.0.0 release

diverges further for each level of nesting
I didn't follow what you mean?

Just that the difference in syntax grows the more levels of nesting you have.

what do you think of optional return types that support subsequent accessors without null check?

Well we need some sort of syntactic sugar to not it make so painful, that's for sure.

The problem with returning Optional is that it creates a tri-state (optional-present, optional-empty, and null). While we can trust that the core implementation of StructuredConfigProperties never returns null, other user-provided interfaces might not be so well behaved.

Could also define a "get or default" overload of the form: getStructured(String name, StructuredConfigProperties defaultProperties)

If we combine this with a EMPTY_CONFIG constant, walking the tree becomes:

config.getStructured("common", EMPTY_CONFIG).getBoolean("default-enabled", true);

Not bad.

if we do some initial prototyping and like it, we could do a one-time cutover in the 3.0.0 release

That's true, but I doubt that the instrumentation config API will be stable in time for 3.0.0 which I assume is happening at the end of the year. So then we'll need to consider whether we're comfortable depending on an experimental API, or should wait till 4.0.0 for a cutover.

I see these as feasible variants

Optional:

config.getStructured("common") .flatMap(c -> c.getStructured("foo") .flatMap(c -> c.getBoolean("default-enabled", true));

getStructured never returns null (but maybe has an isPresent method) - I like that one

config.getStructured("common").getStructured("foo").getBoolean("default-enabled", true);

It's true that custom implementations may still return null (for any of the above) - but they could also throw exceptions.

Optional:

The SDK has no precedent for returning optional instead of null. I believe that was an intentional design decision long ago, and I'm not interested in breaking symmetry.

getStructured never returns null (but maybe has an isPresent method) - I like that one

Users need a way to tell determine the when a key was present and empty vs. null, so never returning null (or some equivalent) is not an option. We could add isPresent, but that's the same as calling getString("foo") != null so no improvement in user calling ergonomics.

PR here to add getStructured default method, which I think has good ergonomics and is symmetric with existing patterns in opentelemetry-java: open-telemetry/opentelemetry-java#6759

So it would look like this for a real example - looks too heavy IMO:

structuredConfigProps .getStructured("otel", empty()) .getStructured("instrumentation", empty()) .getStructured("http", empty()) .getStructured("client", empty()) .getMap("capture-request-headers"))

There's abstractions in place to read common properties so that only a few places in code have to do deep tree walking. And no code will need to read the .getStructured("otel", empty()).getStructured("instrumentation", empty()) steps since those happen automatically.

conventions/src/main/kotlin/otel.java-conventions.gradle.kts

zeitlinger · 2024-09-27T12:04:06Z

...api/src/main/java/io/opentelemetry/javaagent/extension/StructuredConfigPropertiesBridge.java

+ *         string_key: value
+ * </pre>
+ */
+final class StructuredConfigPropertiesBridge implements ConfigProperties {


would make sense to instrumentation-api-incubator so that in can be used in extensions

I wasn't sure where to include this. @open-telemetry/java-instrumentation-maintainers is this or instrumentation-api-incubator the right thing for this type of thing?

javaagent modules can't be used outside the agent - please correct me if I'm wrong @laurit

the current location seems good

would make sense to instrumentation-api-incubator so that in can be used in extensions

@zeitlinger maybe what you really want is open-telemetry/opentelemetry-java#6549?

I want to use the bridge in the starter at some point here:

opentelemetry-java-instrumentation/instrumentation/spring/spring-boot-autoconfigure/build.gradle.kts

Line 65 in 428aa28

implementation(project(":sdk-autoconfigure-support"))

But maybe that's a next step.

zeitlinger · 2024-09-27T12:23:43Z

...java/io/opentelemetry/javaagent/bootstrap/internal/StructuredConfigPropertiesBridgeTest.java

+import java.util.Map;
+import org.junit.jupiter.api.Test;
+
+class StructuredConfigPropertiesBridgeTest {


I see these as feasible variants

Optional:

config.getStructured("common") .flatMap(c -> c.getStructured("foo") .flatMap(c -> c.getBoolean("default-enabled", true));

getStructured never returns null (but maybe has an isPresent method) - I like that one

config.getStructured("common").getStructured("foo").getBoolean("default-enabled", true);

It's true that custom implementations may still return null (for any of the above) - but they could also throw exceptions.

javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/AgentListener.java

...api/src/main/java/io/opentelemetry/javaagent/extension/StructuredConfigPropertiesBridge.java

…y-java-instrumentation into support-declarative-config

trask

lgtm

dependencyManagement/build.gradle.kts

…y-java-instrumentation into support-declarative-config

trask · 2024-10-14T21:23:34Z

...api/src/main/java/io/opentelemetry/javaagent/extension/StructuredConfigPropertiesBridge.java

+  @Nullable
+  @Override
+  public Duration getDuration(String propertyName) {
+    Long millis = getPropertyValue(propertyName, StructuredConfigProperties::getLong);
+    if (millis == null) {
+      return null;
+    }
+    return Duration.ofMillis(millis);
+  }


is this intentionally scoping down and not handling durations with units like DefaultConfigProperties?

Yes. All durations in the declarative config schema are integer types assumed to be ms. There is no ability to specify the 1s, 1h, 1ms at this time.

Support declarative configuration

983515c

jack-berg requested a review from a team September 16, 2024 20:49

jack-berg marked this pull request as draft September 16, 2024 20:49

github-actions bot added the test native This label can be applied to PRs to trigger them to run native tests label Sep 16, 2024

jack-berg mentioned this pull request Sep 16, 2024

Add declarative config support for RuleBasedRoutingSampler open-telemetry/opentelemetry-java-contrib#1440

Merged

Implement StructuredConfigPropertiesBridge

a1d69f4

jack-berg commented Sep 20, 2024

View reviewed changes

Fix class loading stuff

ee9cf67

jack-berg mentioned this pull request Sep 20, 2024

Demosntrate declarative config 0.3 with otel java agent open-telemetry/opentelemetry-java-examples#492

Open

zeitlinger reviewed Sep 27, 2024

View reviewed changes

jeanbisutti reviewed Sep 27, 2024

View reviewed changes

javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/AgentListener.java Show resolved Hide resolved

steverao reviewed Oct 2, 2024

View reviewed changes

...api/src/main/java/io/opentelemetry/javaagent/extension/StructuredConfigPropertiesBridge.java Outdated Show resolved Hide resolved

steverao reviewed Oct 2, 2024

View reviewed changes

...api/src/main/java/io/opentelemetry/javaagent/extension/StructuredConfigPropertiesBridge.java Outdated Show resolved Hide resolved

jack-berg mentioned this pull request Oct 2, 2024

Add getStructured default method, add empty StructuredConfigProperties open-telemetry/opentelemetry-java#6759

Merged

jack-berg added 4 commits October 2, 2024 15:04

PR feedback

2fffe26

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

a2534ba

…y-java-instrumentation into support-declarative-config

Simplify tree walking

3e2e6f1

remote extra blank lines

63256f7

trask reviewed Oct 10, 2024

View reviewed changes

trask reviewed Oct 11, 2024

View reviewed changes

dependencyManagement/build.gradle.kts Outdated Show resolved Hide resolved

jack-berg mentioned this pull request Oct 11, 2024

Prevent kafka client-id from being emitted open-telemetry/opentelemetry-java#6781

Open

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

e76bbac

…y-java-instrumentation into support-declarative-config

jack-berg marked this pull request as ready for review October 14, 2024 14:29

jack-berg requested a review from a team as a code owner October 14, 2024 14:29

trask reviewed Oct 14, 2024

View reviewed changes

trask approved these changes Oct 14, 2024

View reviewed changes

trask merged commit 780cdf4 into open-telemetry:main Oct 16, 2024
56 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support declarative configuration #12265

Support declarative configuration #12265

jack-berg commented Sep 16, 2024 •

edited

Loading

jack-berg Sep 20, 2024

jack-berg Sep 20, 2024

trask Sep 23, 2024

jack-berg Sep 23, 2024

trask Sep 23, 2024

jack-berg Sep 23, 2024 •

edited

Loading

zeitlinger Sep 27, 2024

jack-berg Oct 2, 2024

zeitlinger Oct 14, 2024

jack-berg Oct 14, 2024 •

edited

Loading

zeitlinger Sep 27, 2024

jack-berg Sep 30, 2024

zeitlinger Oct 1, 2024

trask Oct 2, 2024

zeitlinger Oct 14, 2024

zeitlinger Sep 27, 2024

trask left a comment

trask Oct 14, 2024

jack-berg Oct 14, 2024

Support declarative configuration #12265

Support declarative configuration #12265

Conversation

jack-berg commented Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jack-berg Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jack-berg Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trask left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jack-berg commented Sep 16, 2024 •

edited

Loading

jack-berg Sep 23, 2024 •

edited

Loading

jack-berg Oct 14, 2024 •

edited

Loading