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.

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

Alter instrumentation suppression behavior #11640

Merged
merged 3 commits into from
Jul 10, 2024
Merged

Alter instrumentation suppression behavior #11640

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6ab8f65
Alter instrumentation suppression behavior
laurit Jun 20, 2024
cc554ef
update test
laurit Jun 20, 2024
2c80d23
add comment
laurit Jul 9, 2024
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
4 changes: 4 additions & 0 deletions ...main/java/io/opentelemetry/javaagent/extension/instrumentation/InstrumentationModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ public abstract class InstrumentationModule implements Ordered {
* InstrumentationModule} must have a default constructor (for SPI), so they have to pass the
* instrumentation names to the super class constructor.
*
* <p>When enabling or disabling the instrumentation module configuration property that
* corresponds to the main instrumentation name is considered first, after that additional
* instrumentation names are considered in the order they are listed here.
*
* <p>The instrumentation names should follow several rules:
*
* <ul>
Expand Down
14 changes: 4 additions & 10 deletions javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/config/AgentConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,14 @@ public final class AgentConfig {

public static boolean isInstrumentationEnabled(
ConfigProperties config, Iterable<String> instrumentationNames, boolean defaultEnabled) {
// If default is enabled, we want to enable individually,
// if default is disabled, we want to disable individually.
Comment on lines -14 to -15
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe update the comment to reflect this change? something like use default config only when system properties are not set.

boolean anyEnabled = defaultEnabled;
for (String name : instrumentationNames) {
String propertyName = "otel.instrumentation." + name + ".enabled";
boolean enabled = config.getBoolean(propertyName, defaultEnabled);

if (defaultEnabled) {
anyEnabled &= enabled;
} else {
anyEnabled |= enabled;
Boolean enabled = config.getBoolean(propertyName);
if (enabled != null) {
return enabled;
}
}
return anyEnabled;
return defaultEnabled;
}

public static boolean isDebugModeEnabled(ConfigProperties config) {
Expand Down
44 changes: 35 additions & 9 deletions ...gent-tooling/src/test/java/io/opentelemetry/javaagent/tooling/config/AgentConfigTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,14 @@ class AgentConfigTest {
@ArgumentsSource(InstrumentationEnabledParams.class)
void testIsInstrumentationEnabled(
@SuppressWarnings("unused") String description,
boolean firstEnabled,
boolean secondEnabled,
Boolean firstEnabled,
Boolean secondEnabled,
boolean defaultEnabled,
boolean expected) {

ConfigProperties config = mock(ConfigProperties.class);
when(config.getBoolean("otel.instrumentation.first.enabled", defaultEnabled))
.thenReturn(firstEnabled);
when(config.getBoolean("otel.instrumentation.second.enabled", defaultEnabled))
.thenReturn(secondEnabled);
when(config.getBoolean("otel.instrumentation.first.enabled")).thenReturn(firstEnabled);
when(config.getBoolean("otel.instrumentation.second.enabled")).thenReturn(secondEnabled);

assertEquals(
expected,
Expand All @@ -49,13 +47,41 @@ public Stream<? extends Arguments> provideArguments(ExtensionContext context) {
return Stream.of(
Arguments.of(
"enabled by default, both instrumentations are off", false, false, true, false),
Arguments.of("enabled by default, one instrumentation is on", true, false, true, false),
Arguments.of("enabled by default, first instrumentation is on", true, null, true, true),
Arguments.of("enabled by default, second instrumentation is on", null, true, true, true),
Arguments.of("enabled by default, both instrumentations are on", true, true, true, true),
Arguments.of(
"enabled by default, first instrumentation is off, second is on",
false,
true,
true,
false),
Arguments.of(
"enabled by default, first instrumentation is on, second is off",
true,
false,
true,
true),
Arguments.of("enabled by default", null, null, true, true),
Arguments.of(
"disabled by default, both instrumentations are off", false, false, false, false),
Arguments.of("disabled by default, one instrumentation is on", true, false, false, true),
Arguments.of("disabled by default, first instrumentation is on", true, null, false, true),
Arguments.of(
"disabled by default, second instrumentation is on", null, true, false, true),
Arguments.of("disabled by default, both instrumentation are on", true, true, false, true),
Arguments.of(
"disabled by default, first instrumentation is off, second is on",
false,
true,
false,
false),
Arguments.of(
"disabled by default, both instrumentation are on", true, true, false, true));
"disabled by default, first instrumentation is on, second is off",
true,
false,
false,
true),
Arguments.of("disabled by default", null, null, false, false));
}
}
}
Loading