-
Notifications
You must be signed in to change notification settings - Fork 855
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
GlobalOpenTelemetry trigger of autoconfiguration is opt-in #5010
Merged
jack-berg
merged 2 commits into
open-telemetry:main
from
jack-berg:global-opentelemetry-autoconfigure
Dec 14, 2022
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
68 changes: 68 additions & 0 deletions
68
api/all/src/main/java/io/opentelemetry/api/internal/ConfigUtil.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.api.internal; | ||
|
||
import java.util.Locale; | ||
import java.util.Map; | ||
import javax.annotation.Nullable; | ||
|
||
/** | ||
* Configuration utilities. | ||
* | ||
* <p>This class is internal and is hence not for public use. Its APIs are unstable and can change | ||
* at any time. | ||
*/ | ||
public final class ConfigUtil { | ||
|
||
private ConfigUtil() {} | ||
|
||
/** | ||
* Return the system property or environment variable for the {@code key}. | ||
* | ||
* <p>Normalize the {@code key} using {@link #normalizePropertyKey(String)}. Match to system | ||
* property keys also normalized with {@link #normalizePropertyKey(String)}. Match to environment | ||
* variable keys normalized with {@link #normalizeEnvironmentVariableKey(String)}. System | ||
* properties take priority over environment variables. | ||
* | ||
* @param key the property key | ||
* @return the system property if not null, or the environment variable if not null, or null | ||
*/ | ||
@Nullable | ||
public static String getString(String key) { | ||
String normalizedKey = normalizePropertyKey(key); | ||
String systemProperty = | ||
System.getProperties().entrySet().stream() | ||
.filter(entry -> normalizedKey.equals(normalizePropertyKey(entry.getKey().toString()))) | ||
.map(entry -> entry.getValue().toString()) | ||
.findFirst() | ||
.orElse(null); | ||
if (systemProperty != null) { | ||
return systemProperty; | ||
} | ||
return System.getenv().entrySet().stream() | ||
.filter(entry -> normalizedKey.equals(normalizeEnvironmentVariableKey(entry.getKey()))) | ||
.map(Map.Entry::getValue) | ||
.findFirst() | ||
.orElse(null); | ||
} | ||
|
||
/** | ||
* Normalize an environment variable key by converting to lower case and replacing "_" with ".". | ||
*/ | ||
public static String normalizeEnvironmentVariableKey(String key) { | ||
return key.toLowerCase(Locale.ROOT).replace("_", "."); | ||
} | ||
|
||
/** Normalize a property key by converting to lower case and replacing "-" with ".". */ | ||
public static String normalizePropertyKey(String key) { | ||
return key.toLowerCase(Locale.ROOT).replace("-", "."); | ||
} | ||
|
||
/** Returns defaultValue if value is null, otherwise value. This is an internal method. */ | ||
public static <T> T defaultIfNull(@Nullable T value, T defaultValue) { | ||
return value == null ? defaultValue : value; | ||
} | ||
} |
59 changes: 59 additions & 0 deletions
59
api/all/src/test/java/io/opentelemetry/api/internal/ConfigUtilTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.api.internal; | ||
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
|
||
import org.junit.jupiter.api.Test; | ||
import org.junitpioneer.jupiter.SetSystemProperty; | ||
|
||
/** Relies on environment configuration in {@code ./api/all/build.gradle.kts}. */ | ||
class ConfigUtilTest { | ||
|
||
@Test | ||
@SetSystemProperty(key = "config.key", value = "system") | ||
void getString_SystemPropertyPriority() { | ||
assertThat(ConfigUtil.getString("config.key")).isEqualTo("system"); | ||
assertThat(ConfigUtil.getString("config-key")).isEqualTo("system"); | ||
assertThat(ConfigUtil.getString("other.config.key")).isEqualTo(null); | ||
} | ||
|
||
@Test | ||
@SetSystemProperty(key = "CONFIG-KEY", value = "system") | ||
void getString_SystemPropertyNormalized() { | ||
assertThat(ConfigUtil.getString("config.key")).isEqualTo("system"); | ||
assertThat(ConfigUtil.getString("config-key")).isEqualTo("system"); | ||
assertThat(ConfigUtil.getString("other.config.key")).isEqualTo(null); | ||
} | ||
|
||
@Test | ||
void getString_EnvironmentVariable() { | ||
assertThat(ConfigUtil.getString("config.key")).isEqualTo("environment"); | ||
assertThat(ConfigUtil.getString("other.config.key")).isEqualTo(null); | ||
} | ||
|
||
@Test | ||
void normalizeEnvironmentVariable() { | ||
assertThat(ConfigUtil.normalizeEnvironmentVariableKey("CONFIG_KEY")).isEqualTo("config.key"); | ||
assertThat(ConfigUtil.normalizeEnvironmentVariableKey("config_key")).isEqualTo("config.key"); | ||
assertThat(ConfigUtil.normalizeEnvironmentVariableKey("config-key")).isEqualTo("config-key"); | ||
assertThat(ConfigUtil.normalizeEnvironmentVariableKey("configkey")).isEqualTo("configkey"); | ||
} | ||
|
||
@Test | ||
void normalizePropertyKey() { | ||
assertThat(ConfigUtil.normalizePropertyKey("CONFIG_KEY")).isEqualTo("config_key"); | ||
assertThat(ConfigUtil.normalizePropertyKey("CONFIG.KEY")).isEqualTo("config.key"); | ||
assertThat(ConfigUtil.normalizePropertyKey("config-key")).isEqualTo("config.key"); | ||
assertThat(ConfigUtil.normalizePropertyKey("configkey")).isEqualTo("configkey"); | ||
} | ||
|
||
@Test | ||
void defaultIfnull() { | ||
assertThat(ConfigUtil.defaultIfNull("val1", "val2")).isEqualTo("val1"); | ||
assertThat(ConfigUtil.defaultIfNull(null, "val2")).isEqualTo("val2"); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
22 changes: 0 additions & 22 deletions
22
...ns/autoconfigure-spi/src/main/java/io/opentelemetry/sdk/autoconfigure/spi/ConfigUtil.java
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
is null the right response here, rather than the no-op implementation?
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.
Looking at the caller of this...I think we might be doing the wrong thing right now. If this method returns null, we're calling
set
with the no-op impl. That will mean (I think?) that we will always end up returning the no-op, because of the check in theset
method. I think this is wrong because we do want people to be able toset
after calling a uninitializedget
.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 is correct. Here's a summary of the behavior as of right now on
main
:GlobalOpenTelemetry.get()
is called beforeGlobalOpenTelemetry.set()
, anOpenTelemetry
instance is resolved and set usingGlobalOpenTelemetry.set()
.GlobalOpenTelemetry
will be set to an autoconfigured instance ofOpenTelemetrySdk
.GlobalOpenTelemetry
will be set toOpenTelemetry.noop()
.I'm not so sure. The current behavior is inconvenient because it forces users to either call
GlobalOpenTelemetry.set
OR have autoconfiguration do so before any call toGlobalOpenTelemetry.get
. Trying to callGlobalOpenTelemetry.set
later triggers an exception which can only be resolved by adjusting app initialization ordering. But the benefit of this is thatGlobalOpenTelemetry.get
always returns the same value, regardless of the order in which its called. I.e. you can't have one caller receive aOpenTelemetry.noop()
and another caller receive an autoconfiguredOpenTelemetrySdk
. This is a nice invariant to be able to rely on.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.
Hm. I can see that side of things as well (wanting in invariant). I guess it boils down to: in the case of incorrect initialization ordering, do you want a) nothing to be emitted from the SDK at all or b) at least some things emitted, from the instruments/tracers/etc that are initialized after the SDK initialization has taken place.
I don't know that there's an easy answer to this one. 🤔
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.
Yup it's a good question and is kind of a design philosophy question between fail fast or allow partial success.
I'm prefer to the fail fast because I think the absence of all telemetry provides a really strong signal to go and fix the issue.
If we do want to change this behavior should probably do it in a different PR to separate concerns.
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'm not sure I'll be able to make sure no one will call get before I create the OpenTelemetry object.
I will need to doublecheck this 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.
You don't need to. If you rely on
GlobalOpenTelemetry.get()
callingAutoConfiguredOpenTelemetrySdk.initialize()
, you will just need to opt in to that behavior with an environment variable or system property.The current behavior should still be possible, but it shouldn't come at the expense of other scenarios that don't want the side affect. It makes sense to the
AutoConfiguredOpenTelemetrySdk.initialize()
side affect opt-in rather than opt-out because autoconfigure users should be comfortable adding environment variables / system properties so it shouldn't be a problem be a big lift to add one more.