Jdbc declarative config params

[zeitlinger]

I tried out using the declarative config API directly as discussed in #14767

It feels quite verbose - but let's use it as a start for the discussion

[zeitlinger]

@trask please have a look

[jeanbisutti]

Are there tests for declarative configuration applied to the JDBC instrumentation?

[zeitlinger]

to early for that - I first want to discuss if this approach is better than #14767

[jeanbisutti]

(minor) The else could be removed in this way:

if (!openTelemetry instanceof ExtendedOpenTelemetry) {
   return ConfigPropertiesUtil.getBoolean(
          "otel.instrumentation.jdbc.experimental.transaction.enabled", defaultEnabled);
}

[laurit]

It feels quite verbose - but let's use it as a start for the discussion

If you had

  private static boolean getBoolean(
      ExtendedOpenTelemetry extendedOpenTelemetry, boolean defaultValue, String... path) {
    DeclarativeConfigProperties instrumentationConfig =
        extendedOpenTelemetry.getConfigProvider().getInstrumentationConfig();
    if (instrumentationConfig == null) {
      return defaultValue;
    }

    DeclarativeConfigProperties current = instrumentationConfig;
    for (int i = 0; i < path.length - 1; i++) {
      current = current.getStructured(path[i], empty());
    }
    return current.getBoolean(path[path.length - 1], defaultValue);
  }

then you could write

      return getBoolean(
          (ExtendedOpenTelemetry) openTelemetry,
          false,
          "jdbc",
          "experimental",
          "capture-query-parameters");

[zeitlinger]

It feels quite verbose - but let's use it as a start for the discussion

If you had

  private static boolean getBoolean(
      ExtendedOpenTelemetry extendedOpenTelemetry, boolean defaultValue, String... path) {
    DeclarativeConfigProperties instrumentationConfig =
        extendedOpenTelemetry.getConfigProvider().getInstrumentationConfig();
    if (instrumentationConfig == null) {
      return defaultValue;
    }

    DeclarativeConfigProperties current = instrumentationConfig;
    for (int i = 0; i < path.length - 1; i++) {
      current = current.getStructured(path[i], empty());
    }
    return current.getBoolean(path[path.length - 1], defaultValue);
  }

then you could write

      return getBoolean(
          (ExtendedOpenTelemetry) openTelemetry,
          false,
          "jdbc",
          "experimental",
          "capture-query-parameters");

yes, this would be middle ground between the declarative config bridge in #14767 and the current PR.

I think I still lean towards using the bridge directly - because it saves us from accidental spelling mistakes or the need to test this new method.

[trask]

I like the declarative config syntax, I think it's ok that it's a bit verbose, but it's our forward looking config API and so I think it's ok to lean into it

I don't mind the if/else, but if we are going to bridge, I'd prefer to bridge in the other direction where we back the new declarative config API with a fake model based on the system properties, so that instrumentation only needs to use the official API

(this would actually be very nice I think if it works, to push instrumentation into the new config API)

[zeitlinger]

I like the idea - created #15339