The one true configuration bridge

[trask]

The first step towards

• One configuration API to rule them all #15599

[zeitlinger]

Very similar to what I did in #15622 - I focused more on the case where InstrumentationConfig is not available today - so it's mostly complementary (but I didn't check out the big PR yet)

[zeitlinger]

"java", empty()) will be there for every call, so I proposed to get rid of it in #15622

List<String> value =
        ConfigProvider.noop()
            .get("http")
            .get("client")
            .getScalarList("known-methods", String.class, Collections.singletonList("GET"));
    assertThat(value).containsExactly("GET");

[zeitlinger]

OK, we do need to be able to access general as well - so maybe just add another getJava for the common case

[trask]

I want to try leaning into using the Declarative Configuration API directly. If it has usability issues let's raise them to @jack-berg.

[jack-berg]

I don't like that in many cases empty() will be present, but the motivation is that null is different from present and empty.

I'm open to ideas about adding syntactic sugar to facilitate this common task of walking the tree.

Maybe something like:

List<String> value = config.walk("java.http.client", empty()).getScalarList("known-methods", String.class, singletonList("GET"));

Where walk(String dotDelimetedPath, DeclarativeConfigProperties defaultValue) splits the path on ".", and calls getStructured(segment, defaultValue) for each segment.

Or maybe just:

List<String> value = config.walk("java").walk("http").walk("client").getScalarList("known-methods", String.class, singletonList("GET"));

Where walk is an alias for getStructured(String name, empty())

[zeitlinger]

I've created open-telemetry/opentelemetry-java#7923 to address this

[zeitlinger]

Good to merge from my point of view

[trask]

need to think about which instrumentation configurations are stable (e.g. as opposed to "graphql/development")

the downside to adding "/development" is that then we have to deal with deprecating the "/development" node later

maybe it's ok to be more aggressive on marking configurations as stable

cc @laurit

[zeitlinger]

We had already discussed that we want to keep accepting the development suffix even after stabilization

[zeitlinger]

An argument for stable is: the whole instrumentation block is already development - but that's more a formal argument

[trask]

We had already discussed that we want to keep accepting the development suffix even after stabilization

yeah, but it's not clear yet if this will be responsibility of each instrumentation to check both, or if this will be handled transparently in the SDK

[laurit]

maybe it's ok to be more aggressive on marking configurations as stable

I think this is a good idea. With the current approach most of our instrumentations and their options will be perpetually experimental which isn't really useful.

[trask]

how do folks like this Configuration class encapsulation as a first stab? the configuration code is kind of ugly (and hard to read) when strewn about

I think we've discussed this kind of thing before where we would ideally generate something like this from a declarative configuration definition somewhat similar to semconv generation

[zeitlinger]

Yes, it's hard to read - I had to check the implementation. Why not assign the default value in the config class?

And yes, it's a great opportunity to do code generation from the metadata yaml file.
I think this could be done in a follow up pr.
@jaydeluca fyi

[trask]

Why not assign the default value in the config class?

oh, true, I forgot that default values are encoded in configuration schema now

though I'm not sure if defaultBehavior is structured enough to use it for code generation

cc @jack-berg

[jack-berg]

oh, true, I forgot that default values are encoded in configuration schema now

Encoded is a stretch.. We define the behavior, but its up to the implementation to apply that behavior.

And yes, it's a great opportunity to do code generation from the metadata yaml file.

JSON schema does have a default keyword: https://json-schema.org/understanding-json-schema/reference/annotations

It just doesn't make sense to use in declarative config because quite often we have to define defaults which cannot be encoded in a value.

Java instrumentation might be able to use the default annotation, and leverage it in codegen, if it chooses to express the schema for its instrumentations in json schema.

[laurit]

I don't particularly like it because the original code was simpler. We could consider building our own utilities with simpler api that use the declarative config api under the hood. 99% of time we are reading simple booleans or strings anyway.
We could have something like

class AgentConfiguration {
  private final DeclarativeConfigProperties javaConfig;

  private AgentConfiguration(DeclarativeConfigProperties javaConfig) {
    this.javaConfig = javaConfig;
  }

  // could be a singleton
  static AgentConfiguration get() {
    OpenTelemetry openTelemetry = GlobalOpenTelemetry.get();
    DeclarativeConfigProperties javaConfig = empty();
    if (openTelemetry instanceof ExtendedOpenTelemetry) {
      ExtendedOpenTelemetry extendedOpenTelemetry = (ExtendedOpenTelemetry) openTelemetry;
      DeclarativeConfigProperties instrumentationConfig =
          extendedOpenTelemetry.getConfigProvider().getInstrumentationConfig();
      if (instrumentationConfig != null) {
        javaConfig = instrumentationConfig.getStructured("java", empty());
      }
    }

    return new AgentConfiguration(javaConfig);
  }

  boolean getBoolean(boolean defaultValue, String... key) {
    if (key.length == 0) {
      return defaultValue;
    }

    DeclarativeConfigProperties node = javaConfig;
    for (int i = 0; i < key.length - 2; i++) {
      node = node.getStructured(key[i], empty());
    }
    return node.getBoolean(key[key.length - 1], defaultValue);
  }
}

and you could use it with

AgentConfiguration.get().getBoolean(true, "graphql", "query_sanitizer", "enabled");

could also add

    boolean getBoolean(String key, boolean defaultValue) {
      return getBoolean(defaultValue, key.split("\\."));
    }

then you could use it with

AgentConfiguration.get().getBoolean("graphql.query_sanitizer.enabled", true);

which would be pretty much the same as the exiting code

[trask]

This part:

    OpenTelemetry openTelemetry = GlobalOpenTelemetry.get();
    DeclarativeConfigProperties javaConfig = empty();
    if (openTelemetry instanceof ExtendedOpenTelemetry) {
      ExtendedOpenTelemetry extendedOpenTelemetry = (ExtendedOpenTelemetry) openTelemetry;
      DeclarativeConfigProperties instrumentationConfig =
          extendedOpenTelemetry.getConfigProvider().getInstrumentationConfig();
      if (instrumentationConfig != null) {
        javaConfig = instrumentationConfig.getStructured("java", empty());
      }
    }

should soon be simplified to:

GlobalOpenTelemetry.get().getConfigProvider().getInstrumentationConfig().getStructured("java", empty())

(with stabilization of getConfigProvider and open-telemetry/opentelemetry-java#7920)

if the other part is still too complex for our preference, I'd like to try to improve the Declarative Configuration API ergonomics (instead of adding our own wrapper)

e.g. @zeitlinger has a proposal along these lines: open-telemetry/opentelemetry-java#7923

are there other things from your perspective that would make using the Declarative Configuration API directly better for us?

[jack-berg]

AgentConfiguration.get().getBoolean("graphql.query_sanitizer.enabled", true);

With open-telemetry/opentelemetry-java#7920 and open-telemetry/opentelemetry-java#7923 (and eventual stabilization of these APIs) we'll end up with something similar:

GlobalOpenTelemetry.getConfigProvider().getInstrumentationConfig()
  .get("graphql)
  .get("query_sanitizer")
  .getBoolean("enabled", true);

And the agent could provide instrumentation helpers for the repetitive GlobalOpenTelemetry.getConfigProvider().getInstrumentationConfig() part.

[zeitlinger]

Actually it would be

GlobalOpenTelemetry.get().getConfigProvider().getInstrumentationConfig()
  .get("java")
  .get("graphql")
  .get("query_sanitizer")
  .getBoolean("enabled", true);

@jack-berg difference to what you wrote

• GlobalOpenTelemetry.getConfigProvider() doesn't exist yet (but I think you implied that it would be added)
• .get("java") was missing

This inspired me to create open-telemetry/opentelemetry-java#7927 so it can become

GlobalOpenTelemetry.get().getConfigProvider()
  .get("graphql")
  .get("query_sanitizer")
  .getBoolean("enabled", true);

• removes .getInstrumentationConfig()
• removes `.get("java")

[trask]

@laurit I'm going to go ahead and merge so I can submit a couple of follow-ups, but feel free to leave comments here and I'll follow-up on them

[laurit]

Do we need to add some way to customize this to make it usable in vendor distros that could need to map their of configuration properties?

[zeitlinger]

In my current PoC, I have a mapper class that some common default properties do declarative config, based on what was specified as system properties:

https://github.com/grafana/grafana-opentelemetry-java/blob/c0e5e7f7093fbd341eee29eb9f2342d5a6d1d9b1/custom/src/main/java/com/grafana/extensions/DeclarativeConfigPropertyCustomizer.java#L13

It leaves enough wiggle room for vendor conventions.