Skip to content

Register config-inversion for InstrumenterConfig #9953

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Nov 13, 2025
+24 −14
Merged

Register config-inversion for InstrumenterConfig #9953

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7966580
Register ConfigInversion netric collector for InstrumenterConfig
mcculls Nov 13, 2025
c919872
Document DBCP2 and HIKARI integration flags
mcculls Nov 13, 2025
ea05e22
comment
mcculls Nov 13, 2025
0e726e3
Add NoOpConfigInversionMetricCollector to expected class list in GraalVM
mcculls Nov 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ...og/trace/instrumentation/graal/nativeimage/NativeImageGeneratorRunnerInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ public static void onEnter(@Advice.Argument(value = 0, readOnly = false) String[
+ "datadog.trace.api.telemetry.ConfigInversionMetricCollectorProvider:rerun,"
+ "datadog.trace.api.telemetry.ConfigInversionMetricCollectorImpl:build_time,"
+ "datadog.trace.api.telemetry.ConfigInversionMetricCollectorImpl$ConfigInversionMetric:build_time,"
+ "datadog.trace.api.telemetry.NoOpConfigInversionMetricCollector:build_time,"
+ "datadog.trace.api.telemetry.OtelEnvMetricCollectorImpl:build_time,"
+ "datadog.trace.api.profiling.ProfilingEnablement:build_time,"
+ "datadog.trace.bootstrap.config.provider.ConfigConverter:build_time,"
Expand Down
10 changes: 10 additions & 0 deletions internal-api/src/main/java/datadog/trace/api/InstrumenterConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@
import static datadog.trace.util.CollectionUtils.tryMakeImmutableSet;

import datadog.trace.api.profiling.ProfilingEnablement;
import datadog.trace.api.telemetry.ConfigInversionMetricCollectorImpl;
import datadog.trace.api.telemetry.ConfigInversionMetricCollectorProvider;
import datadog.trace.api.telemetry.OtelEnvMetricCollectorImpl;
import datadog.trace.api.telemetry.OtelEnvMetricCollectorProvider;
import datadog.trace.bootstrap.config.provider.ConfigProvider;
Expand Down Expand Up @@ -107,6 +109,14 @@
* @see Config for other configurations
*/
public class InstrumenterConfig {
static {
// skip registration when building native-images as telemetry is not available
if (!Platform.isNativeImageBuilder()) {
ConfigInversionMetricCollectorProvider.register(
Copy link
Contributor

@mhlidd mhlidd Nov 13, 2025

Choose a reason for hiding this comment

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

❓ If we register the Collector in InstrumenterConfig, which gets initialized in Config.java, should we remove the registration that already exists in Config.java(ref)?

Copy link
Contributor

@mhlidd mhlidd Nov 13, 2025

Choose a reason for hiding this comment

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

Also, out of curiosity, what is the added benefit of registering in InstrumenterConfig instead of just Config?

Copy link
Contributor Author

@mcculls mcculls Nov 13, 2025

Choose a reason for hiding this comment

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

See #9953 (comment) - without this registration config-inversion is not applied to InstrumenterConfig, which means new integrations don't cause any CI alerts about documenting them - for example the 'hikari' and 'dbcp2' integrations.

(This is because an instance of InstrumenterConfig is created before Config's static initializer runs.)

In the future as Config is broken apart there will hopefully be a better place to do this registration, but for now we have to do this in both InstrumenterConfig and Config to cover a couple of different user-cases.

Copy link
Contributor Author

@mcculls mcculls Nov 13, 2025

Choose a reason for hiding this comment

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

Also I intentionally left the existing registration in Config because a) it doesn't matter which runs first because the implementation is a singleton and b) if someone refactors this in the future then I think it's clearer to have both registrations in the code

Eventually it would be nice have a clear place to register this kind of thing once - but right now there is this intentional split-brain situation between InstrumenterConfig (controls instrumentation, baked into native images and not re-evaluated) and Config (controls non-instrumentation, re-evaluated in native imeges)

ConfigInversionMetricCollectorImpl.getInstance());
}
}

private final ConfigProvider configProvider;

private final boolean triageEnabled;
Expand Down
4 changes: 4 additions & 0 deletions metadata/supported-configurations.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,6 +632,7 @@
"DD_TRACE_DATANUCLEUS_ANALYTICS_SAMPLE_RATE": ["A"],
"DD_TRACE_DATANUCLEUS_ENABLED": ["A"],
"DD_TRACE_DB2_ENABLED": ["A"],
"DD_TRACE_DBCP2_ENABLED": ["A"],
"DD_TRACE_DBM_ENABLED": ["A"],
"DD_TRACE_DBSTATEMENTRULE_ENABLED": ["A"],
"DD_TRACE_DB_CLIENT_SPLIT_BY_HOST": ["A"],
Expand Down Expand Up @@ -746,6 +747,7 @@
"DD_TRACE_HIBERNATE_CORE_ANALYTICS_SAMPLE_RATE": ["A"],
"DD_TRACE_HIBERNATE_CORE_ENABLED": ["A"],
"DD_TRACE_HIBERNATE_ENABLED": ["A"],
"DD_TRACE_HIKARI_ENABLED": ["A"],
"DD_TRACE_HTTPASYNCCLIENT4_LEGACY_TRACING_ENABLED": ["A"],
"DD_TRACE_HTTPASYNCCLIENT5_ENABLED": ["A"],
"DD_TRACE_HTTPASYNCCLIENT_ANALYTICS_ENABLED": ["A"],
Expand Down Expand Up @@ -1471,6 +1473,7 @@
"DD_TRACE_DATANUCLEUS_ANALYTICS_SAMPLE_RATE": ["DD_DATANUCLEUS_ANALYTICS_SAMPLE_RATE"],
"DD_TRACE_DATANUCLEUS_ENABLED": ["DD_TRACE_INTEGRATION_DATANUCLEUS_ENABLED","DD_INTEGRATION_DATANUCLEUS_ENABLED"],
"DD_TRACE_DB2_ENABLED": ["DD_TRACE_INTEGRATION_DB2_ENABLED","DD_INTEGRATION_DB2_ENABLED"],
"DD_TRACE_DBCP2_ENABLED": ["DD_TRACE_INTEGRATION_DBCP2_ENABLED","DD_INTEGRATION_DBCP2_ENABLED"],
"DD_TRACE_DBM_ENABLED": ["DD_TRACE_INTEGRATION_DBM_ENABLED","DD_INTEGRATION_DBM_ENABLED"],
"DD_TRACE_DEFINECLASS_ENABLED": ["DD_TRACE_INTEGRATION_DEFINECLASS_ENABLED","DD_INTEGRATION_DEFINECLASS_ENABLED"],
"DD_TRACE_DIRECTALLOCATION_ENABLED": ["DD_TRACE_INTEGRATION_DIRECTALLOCATION_ENABLED","DD_INTEGRATION_DIRECTALLOCATION_ENABLED"],
Expand Down Expand Up @@ -1542,6 +1545,7 @@
"DD_TRACE_HIBERNATE_CORE_ANALYTICS_SAMPLE_RATE": ["DD_HIBERNATE_CORE_ANALYTICS_SAMPLE_RATE"],
"DD_TRACE_HIBERNATE_CORE_ENABLED": ["DD_TRACE_INTEGRATION_HIBERNATE_CORE_ENABLED","DD_INTEGRATION_HIBERNATE_CORE_ENABLED"],
"DD_TRACE_HIBERNATE_ENABLED": ["DD_TRACE_INTEGRATION_HIBERNATE_ENABLED","DD_INTEGRATION_HIBERNATE_ENABLED"],
"DD_TRACE_HIKARI_ENABLED": ["DD_TRACE_INTEGRATION_HIKARI_ENABLED","DD_INTEGRATION_HIKARI_ENABLED"],
"DD_TRACE_HTTPASYNCCLIENT4_LEGACY_TRACING_ENABLED": ["DD_HTTPASYNCCLIENT4_LEGACY_TRACING_ENABLED"],
"DD_TRACE_HTTPASYNCCLIENT5_ENABLED": ["DD_TRACE_INTEGRATION_HTTPASYNCCLIENT5_ENABLED","DD_INTEGRATION_HTTPASYNCCLIENT5_ENABLED"],
"DD_TRACE_HTTPASYNCCLIENT_ANALYTICS_SAMPLE_RATE": ["DD_HTTPASYNCCLIENT_ANALYTICS_SAMPLE_RATE"],
Expand Down
15 changes: 2 additions & 13 deletions ...ils/src/main/java/datadog/trace/api/telemetry/ConfigInversionMetricCollectorProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,12 @@
package datadog.trace.api.telemetry;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public final class ConfigInversionMetricCollectorProvider {
private static final Logger log =
LoggerFactory.getLogger(ConfigInversionMetricCollectorProvider.class);
private static ConfigInversionMetricCollector INSTANCE = null;
private static ConfigInversionMetricCollector INSTANCE =
NoOpConfigInversionMetricCollector.getInstance();

private ConfigInversionMetricCollectorProvider() {}

public static ConfigInversionMetricCollector get() {
if (INSTANCE == null) {
log.debug(
"ConfigInversionMetricCollector has not been registered. Defaulting to NoOp implementation.");
// Return NoOp implementation for build tasks like instrumentJava that run before
// implementation is registered
return NoOpConfigInversionMetricCollector.getInstance();
}
return INSTANCE;
}

Expand Down
8 changes: 7 additions & 1 deletion ...g-utils/src/main/java/datadog/trace/api/telemetry/NoOpConfigInversionMetricCollector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
package datadog.trace.api.telemetry;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/**
* NOOP implementation of ConfigInversionMetricCollector. Used as a default when the real collector
* is not registered during build tasks like instrumentJava.
Expand All @@ -8,6 +11,9 @@ public final class NoOpConfigInversionMetricCollector implements ConfigInversion
private static final NoOpConfigInversionMetricCollector INSTANCE =
new NoOpConfigInversionMetricCollector();

private static final Logger log =
LoggerFactory.getLogger(NoOpConfigInversionMetricCollector.class);

private NoOpConfigInversionMetricCollector() {}

public static NoOpConfigInversionMetricCollector getInstance() {
Expand All @@ -16,6 +22,6 @@ public static NoOpConfigInversionMetricCollector getInstance() {

@Override
public void setUndocumentedEnvVarMetric(String configName) {
// NOOP - do nothing
log.debug("Environment variable {} is undocumented", configName);
Copy link
Contributor

@mhlidd mhlidd Nov 13, 2025

Choose a reason for hiding this comment

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

What is the circumstance where we would expect to have a NOOP implementation used? I would expect none, and if that's the case, should we log an error message saying that the actual implementation should be registered? 🤔

Copy link
Contributor Author

@mcculls mcculls Nov 13, 2025

Choose a reason for hiding this comment

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

So this PR came about because I was looking at recent debug logs and noticed a bunch of these messages:

[dd.trace 2025-11-13 16:14:37:076 +0000] [main] DEBUG datadog.trace.api.telemetry.ConfigInversionMetricCollectorProvider - ConfigInversionMetricCollector has not been registered. Defaulting to NoOp implementation.
[dd.trace 2025-11-13 16:14:37:076 +0000] [main] DEBUG datadog.trace.api.telemetry.ConfigInversionMetricCollectorProvider - ConfigInversionMetricCollector has not been registered. Defaulting to NoOp implementation.
[dd.trace 2025-11-13 16:14:37:076 +0000] [main] DEBUG datadog.trace.api.telemetry.ConfigInversionMetricCollectorProvider - ConfigInversionMetricCollector has not been registered. Defaulting to NoOp implementation.
... 27 more duplicate log messages ...

I tracked this back to the missing initialization in InstrumenterConfig - but while fixing that I realized that if this happened again (i.e. the "no-op" implementation being used accidentally) then it would be better to have a log including the undocumented config key - as that would point to where the config was being looked up.

So I would argue we still need logging in the "no-op" implementation to help in the future, even if ideally that will never be called. I chose debug as that is less likely to cause disruption and is best suited for this kind of triaging. (I usually reserve use of warn/error for things that users need to know about and can fix themselves.)

Also there may well be situations (build/tooling) where we don't have telemetry, but this debug logging would be useful - especially as we often have debug on in CI.

Copy link
Contributor

@mhlidd mhlidd Nov 13, 2025

Choose a reason for hiding this comment

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

Makes sense, thanks for the info!

}
}