-
Notifications
You must be signed in to change notification settings - Fork 1k
Move one AgentConfig usage to declarative configuration API #15698
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
base: main
Are you sure you want to change the base?
Conversation
| private static void maybeEnableLoggingExporter( | ||
| SdkTracerProviderBuilder builder, ConfigProperties config) { | ||
| if (AgentConfig.isDebugModeEnabled(config)) { | ||
| if (config.getBoolean("otel.javaagent.debug", false)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AutoConfigurationCustomizerProvider implementations are part of the (non-declarative configuration) SDK autoconfiguration, and so will continue to use ConfigProperties
| public static boolean isInstrumentationEnabled( | ||
| ConfigProperties config, Iterable<String> instrumentationNames, boolean defaultEnabled) { | ||
| for (String name : instrumentationNames) { | ||
| String propertyName = "otel.instrumentation." + name + ".enabled"; | ||
| Boolean enabled = config.getBoolean(propertyName); | ||
| if (enabled != null) { | ||
| return enabled; | ||
| } | ||
| } | ||
| return defaultEnabled; | ||
| } | ||
|
|
||
| public static boolean isDebugModeEnabled(ConfigProperties config) { | ||
| return config.getBoolean("otel.javaagent.debug", false); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both of these methods ended up only being called from a single place, so moved them (and their tests) and deleted this file (and AgentConfigTest)
| ExtendedDeclarativeConfigProperties config = | ||
| DeclarativeConfigUtil.get(GlobalOpenTelemetry.get()); | ||
| for (String name : instrumentationNames) { | ||
| String normalizedName = name.replace('-', '_'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renaming usages to use _ is a later PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's needed here?
otherwise you'd need to use
instrumentation/development:
java:
apache-httpclient:
enabled: false
instead of
instrumentation/development:
java:
apache_httpclient:
enabled: false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's right.
I meant that you could also rename the instrumentation to apache_httpclient - not sure that's really better though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kind of like that, it would be nice for the "instrumentation name" to match what's in declarative config yaml
| } | ||
|
|
||
| private static boolean isDebugModeEnabled() { | ||
| return DeclarativeConfigUtil.get(GlobalOpenTelemetry.get()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is problematic because some usages of the debug flag only use system properties - which would be ignored here.
I added these overrides to address that:
Lines 73 to 78 in 92475f1
| .addOverride("otel.instrumentation.common.default-enabled", defaultEnabled(configProvider)) | |
| // these properties are used to initialize the SDK before the configuration file | |
| // is loaded for consistency, we pass them to the bridge, so that they can be read | |
| // later with the same value from the {@link DeclarativeConfigPropertiesBridge} | |
| .addOverride("otel.javaagent.debug", earlyConfig.getBoolean("otel.javaagent.debug", false)) | |
| .addOverride("otel.javaagent.logging", earlyConfig.getString("otel.javaagent.logging")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, I've reverted the portion of this PR related to otel.javaagent.debug
No description provided.