Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extended config properties #5912

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jack-berg
Copy link
Member

Replaces draft #5873.

@open-telemetry/java-instrumentation-maintainers in the 9/21 Java SIG meeting I brought up bringing the file configuration prototype into the java agent, and received feedback that it will be important to specify other java agent properties besides the SDK in a configuration. Examples include accessing jmx configuration, and other standard properties like otel.javaagent.debug=true.

This PR extends file configuration tooling to include access to configuration not part of the configuration schema. Summary:

  • A user specifies both SDK and java agent configuration in a single YAML file, and sets OTEL_CONFIG_FILE to the file path
  • Autoconfigure detects that OTEL_CONFIG_FILE is set and that opentelemetry-sdk-extension-incubator is on the classpath, and:
    • Parses / interprets the config file to configure the SDK
    • Converts the OpenTelemetryConfiguration model to a generic ExtendedConfigProperties representation accessible via AutoConfiguredOpenTelemetrySdk#getConfig()
  • ExtendedConfigProperties implements ConfigProperties and adds some additional methods for accessing nested complex configuration types (nested maps, and lists of maps)
  • The opentelemetry java agent accesses ExtendedConfigProperties and uses it for configuration

Suppose you have a configuration file with contents like:

file_format: "0.1"
tracer_provder:
  processors:
    - batch:
        exporters:
          otlp:
javaagent:
  debug: true
  jmx:
    foo: bar

In java, you use the file to configure the SDK, as well as access the extended "javaagent" portion of the schema as follows:

export OTEL_CONFIG_FILE=/path-to-config.yaml
...
AutoConfiguredOpenTelemetrySdk autoConfiguredSdk = AutoConfiguredOpenTelemetry.builder().build();
ExtendedConfigProperties configProps = (ExtendedConfigProperties) autoConfiguredSdk.getConfig();

ExtendedConfigProperties javaagentProps = configProps.getConfigProperties("javaagent");
Boolean debug = javaagentProps.getBoolean("debug");
ExtendedConfigProperties jmxProps = javaagentProps.getConfigProperties("jmx");
String foo = jmxProps.getString("foo");

assert foo.equals("bar");

// Or use dot notation 
assert configProps.get("javaagent.jmx.foo").equals("bar");
assert configProps.get("javaagent.debug").equals(true);

@jack-berg jack-berg requested a review from a team as a code owner October 12, 2023 19:41
@jack-berg
Copy link
Member Author

PS. opentelemetry-java-examples#227 how this code can be coupled with changes to the contrib RuleBasedRoutingSampler to be able to configure the otel java agent to drop spans matching a particular pattern (like health checks), which is a very popular feature request.

+ " str_key2: str_value2\n"
+ " int_key2: 3\n"
+ " - str_key1: str_value1\n"
+ " int_key1: 2";
Copy link
Member Author

Choose a reason for hiding this comment

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

Check out this test for examples of how to use the API to read complex types defined in a configuration file.

Notably, I've added support for dot notation to allow existing system property / environment variable configuration to be translated to a hierarchical representation in YAML.

For example, the property otel.javaagent.debug would be represented in YAML with:

otel:
  javaagent:
    debug: true

And you could access it using configProperties.getBoolean("otel.javaagent.debug").

@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

Attention: 21 lines in your changes are missing coverage. Please review.

Files Coverage Δ
...onfigure/spi/internal/DefaultConfigProperties.java 94.49% <100.00%> (+0.20%) ⬆️
...nfigure/AutoConfiguredOpenTelemetrySdkBuilder.java 94.47% <100.00%> (+0.20%) ⬆️
...ion/incubator/fileconfig/ConfigurationFactory.java 92.30% <100.00%> (+16.11%) ⬆️
...nfigure/spi/internal/ExtendedConfigProperties.java 0.00% <0.00%> (ø)
...ion/incubator/fileconfig/FileConfigProperties.java 82.88% <82.88%> (ø)

... and 2 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

Copy link
Contributor

@brunobat brunobat left a comment

Choose a reason for hiding this comment

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

We need to clarify:

  • Which properties will work in the file configuration but not as env. vars.
  • We need to make sure all properties can be set through the properties supplier and that the file configuration can be ignored in that case.

void configFile_ExtendedConfigProperties() {
ConfigProperties config =
DefaultConfigProperties.createFromMap(
Collections.singletonMap("OTEL_CONFIG_FILE", configFilePath.toString()));
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to provide a way to deactivate OTEL_CONFIG_FILE config loading if the properties supplier is being used directly in the Autoconfiguration.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is unrelated to this PR. Its already possible to deactivate OTEL_CONFIG_FILE via property suppliers. See the source code for how OTEL_CONFIG_FILE is loaded. Its accessed via ConfigProperties.getString("otel.config.file") using a ConfigProperties instance that is resolved using all the property suppliers and customizers typical in autoconfigure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it is unrelated with the PR but is of the upmost importance and should be clarified. From what I can see in the code, if the file exists, the programatic interface to create the SDK is not available anymore. This is a big problem.

@jack-berg
Copy link
Member Author

We need to clarify:

Which properties will work in the file configuration but not as env. vars.
We need to make sure all properties can be set through the properties supplier and that the file configuration can be ignored in that case.

@brunobat That feedback is orthogonal to this PR. This PR is about allowing complex configuration to be accessible via an ExtendedConfigProperties such that SPI implementations can reach more than just simple types.

@brunobat
Copy link
Contributor

We need to clarify:

Which properties will work in the file configuration but not as env. vars.
We need to make sure all properties can be set through the properties supplier and that the file configuration can be ignored in that case.

@brunobat That feedback is orthogonal to this PR. This PR is about allowing complex configuration to be accessible via an ExtendedConfigProperties such that SPI implementations can reach more than just simple types.

Yes but we need to clarify which of those extended configurations will be available to be configured with Env. Vars.

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.

None yet

2 participants