Skip to content

Simplified Declarative Configuration API, Option A#15671

Merged
trask merged 1 commit intoopen-telemetry:mainfrom
trask:simplified-declarative-config-api-option-a
Dec 16, 2025
Merged

Simplified Declarative Configuration API, Option A#15671
trask merged 1 commit intoopen-telemetry:mainfrom
trask:simplified-declarative-config-api-option-a

Conversation

@trask
Copy link
Copy Markdown
Member

@trask trask commented Dec 16, 2025

Exploring options for us to start using in this repository immediately, and to recommend upstream to the Declarative Configuration API.

  // instrumentation/development:
  //   java:
  //     graphql:
  //       capture_query: true
  //       query_sanitizer:
  //         enabled: true
  //       data_fetcher:
  //         enabled: false
  //       trivial_data_fetcher:
  //         enabled: false
  //       add_operation_name_to_span_name:
  //         enabled: false
  private static final class Configuration {

    private final boolean captureQuery;
    private final boolean sanitizeQuery;
    private final boolean dataFetcherEnabled;
    private final boolean trivialDataFetcherEnabled;
    private final boolean addOperationNameToSpanName;

    Configuration(OpenTelemetry openTelemetry) {
      ExtendedDeclarativeConfigProperties config = DeclarativeConfigUtil.get(openTelemetry);

      this.captureQuery = config.get("graphql").getBoolean("capture_query", true);
      this.sanitizeQuery = config.get("graphql").get("query_sanitizer").getBoolean("enabled", true);
      this.dataFetcherEnabled =
          config.get("graphql").get("data_fetcher").getBoolean("enabled", false);
      this.trivialDataFetcherEnabled =
          config.get("graphql").get("trivial_data_fetcher").getBoolean("enabled", false);
      this.addOperationNameToSpanName =
          config.get("graphql").get("add_operation_name_to_span_name").getBoolean("enabled", false);
    }
  }

@trask trask force-pushed the simplified-declarative-config-api-option-a branch 2 times, most recently from 2097a18 to 65baab2 Compare December 16, 2025 18:28
@trask trask force-pushed the simplified-declarative-config-api-option-a branch from 65baab2 to 145cb50 Compare December 16, 2025 18:33
this.delegate = delegate;
}

public ExtendedDeclarativeConfigProperties get(String name) {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the new method in DeclarativeConfigProperties that is proposed in this PR

@trask trask marked this pull request as ready for review December 16, 2025 19:10
@trask trask requested a review from a team as a code owner December 16, 2025 19:11
@zeitlinger
Copy link
Copy Markdown
Member

I like this variant over B - and in general

  • fits naturally with the default value handling of the existing methods
  • allows to use intermediate variables, e.g. for the graphql node
  • avoids varargs


private DeclarativeConfigUtil() {}

public static ExtendedDeclarativeConfigProperties get(OpenTelemetry openTelemetry) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method also makes the usage more concise - so it's really apart of the proposal I think

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

possibly, I also don't mind the extra few charatcters of

GlobalOpenTelemetry.get().getConfigProvider().getInstrumentationConfig().

over

DeclarativeConfigUtil.get(GlobalOpenTelemetry.get()).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree - but that requires the stabilization of config provider - which will take a while.

@trask
Copy link
Copy Markdown
Member Author

trask commented Dec 16, 2025

  • fits naturally with the default value handling of the existing methods

Option B could replace the existing default value methods

  • allows to use intermediate variables, e.g. for the graphql node

I wonder how useful this is? (and you can always get it via getStructured if needed)

@jack-berg
Copy link
Copy Markdown
Member

I like this over B as well. #15672 (comment)

@trask
Copy link
Copy Markdown
Member Author

trask commented Dec 16, 2025

Cool, can I get an approval on this PR, and I'll move forward with this approach (I have several PRs pending this decision)

I'll also continue to update anything retroactively based on @laurit's feedback.

@zeitlinger
Copy link
Copy Markdown
Member

I slightly prefer option return over the current default value handling - but it's really only slightly.

@trask trask merged commit c621147 into open-telemetry:main Dec 16, 2025
85 checks passed
@trask trask deleted the simplified-declarative-config-api-option-a branch December 16, 2025 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants