Add system properties bridge #15339
Open
Add system properties bridge #15339
Conversation
This file contains hidden or 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
github-actions
bot
added
the
test native
Member
Author
|
@trask I really like this new bridge based on your idea 😄 |
zeitlinger
changed the title
zeitlinger
force-pushed
the
system-properties-bridge
branch
from
3a9e285 to
9ae95f1
Compare
zeitlinger
changed the title
trask
reviewed
Nov 24, 2025
Comment on lines
26
to
28
| private static final boolean isIncubator = isIncubator(); | ||
|
|
||
| private static boolean isIncubator() { |
Member
There was a problem hiding this comment.
maybe supportsDeclarativeConfig? based on comment below it sounds like we'll still need this once it's out of incubation?
Member
Author
There was a problem hiding this comment.
no, when it's out of incubation, we'll have
public static boolean isDeclarativeConfig(OpenTelemetry openTelemetry) {
return openTelemetry instanceof ExtendedOpenTelemetry;
}or
public static boolean isDeclarativeConfig(OpenTelemetry openTelemetry) {
return openTelemetry.getConfigProvider() != null;
}instead of
public static boolean isDeclarativeConfig(OpenTelemetry openTelemetry) {
return isIncubator && openTelemetry instanceof ExtendedOpenTelemetry;
}depending on how things will evolve.
With the name change, it would be
public static boolean isDeclarativeConfig(OpenTelemetry openTelemetry) {
return supportsDeclarativeConfig && openTelemetry instanceof ExtendedOpenTelemetry;
}which is similar in clarity to what we have now - so I'll go with your proposal
Comment on lines
58
to
59
| public static boolean getBoolean( | ||
| OpenTelemetry openTelemetry, boolean defaultValue, String... propertyName) { |
Member
There was a problem hiding this comment.
what about:
public static Optional getBoolean(OpenTelemetry, String...)
then used as:
getBoolean(...).orElse(...)
Member
Author
There was a problem hiding this comment.
I like it, but I want to keep things consistent - so I'll change it for all methods.
The class in internal, so it's allowed.
Comment on lines
+34
to
+39
| // This only happens in OpenTelemetry API instrumentation tests, where an older version of | ||
| // OpenTelemetry API is used that does not have ExtendedOpenTelemetry. | ||
| // Having the incubator module without ExtendedOpenTelemetry class should still return false | ||
| // for those tests to avoid a ClassNotFoundException. |
Member
There was a problem hiding this comment.
This only happens in OpenTelemetry API instrumentation tests
these are tests of OpenTelemetry API in the users class loader, so not following how that affects this class which is in the agent?
Member
Author
There was a problem hiding this comment.
without the check, we get tests where the incubator exists, but is too old to have ExtendedOpenTelemetry:
gradle :instrumentation:opentelemetry-api:opentelemetry-api-1.42:javaagent:incubatorTest
at io.opentelemetry.instrumentation.api.internal.ConfigPropertiesUtil.isDeclarativeConfig(ConfigPropertiesUtil.java:116)
at io.opentelemetry.instrumentation.api.instrumenter.InstrumenterBuilder.buildSpanSuppressor(InstrumenterBuilder.java:374)
at io.opentelemetry.instrumentation.api.instrumenter.Instrumenter.<init>(Instrumenter.java:105)
at io.opentelemetry.instrumentation.api.instrumenter.InstrumenterBuilder.buildInstrumenter(InstrumenterBuilder.java:298)
at io.opentelemetry.instrumentation.api.instrumenter.InstrumenterBuilder.buildInstrumenter(InstrumenterBuilder.java:287)
at io.opentelemetry.instrumentation.testing.TestInstrumenters.<init>(TestInstrumenters.java:41)
at io.opentelemetry.instrumentation.testing.InstrumentationTestRunner.<init>(InstrumentationTestRunner.java:68)
at io.opentelemetry.instrumentation.testing.AgentTestRunner.<init>(AgentTestRunner.java:47)
at io.opentelemetry.instrumentation.testing.AgentTestRunner.<clinit>(AgentTestRunner.java:40)
at io.opentelemetry.instrumentation.testing.junit.AgentInstrumentationExtension.<init>(AgentInstrumentationExtension.java:35)
at io.opentelemetry.instrumentation.testing.junit.AgentInstrumentationExtension.create(AgentInstrumentationExtension.java:39)
at io.opentelemetry.javaagent.instrumentation.opentelemetryapi.v1_42.incubator.logs.LoggerTest.<clinit>(LoggerTest.java:41)
at java.base/jdk.internal.misc.Unsafe.ensureClassInitialized0(Native Method)
at java.base/jdk.internal.misc.Unsafe.ensureClassInitialized(Unsafe.java:1160)
at java.base/java.lang.reflect.Field.acquireOverrideFieldAccessor(Field.java:1200)
at java.base/java.lang.reflect.Field.getOverrideFieldAccessor(Field.java:1169)
at java.base/java.lang.reflect.Field.get(Field.java:444)
at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:184)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
at java.base/java.util.stream.SortedOps$RefSortingSink.end(SortedOps.java:395)
at java.base/java.util.stream.DistinctOps$1$2.end(DistinctOps.java:168)
at java.base/java.util.stream.Sink$ChainedReference.end(Sink.java:261)
at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:510)
at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:151)
at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:174)
at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:596)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
Caused by: java.lang.ClassNotFoundException: io.opentelemetry.api.incubator.ExtendedOpenTelemetry
at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641)
at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188)
at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:526)
... 29 more
Comment on lines
142
to
128
| /** Returns true if the given OpenTelemetry instance supports Declarative Config. */ | ||
| public static boolean isDeclarativeConfig(OpenTelemetry openTelemetry) { | ||
| return isIncubator && openTelemetry instanceof ExtendedOpenTelemetry; | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
is isIncubator needed, since instrumentation-api (unfortunately) already depends on the api-incubator?
Member
Author
There was a problem hiding this comment.
only for the test (see last comment)
zeitlinger
force-pushed
the
system-properties-bridge
branch
from
0138d4f to
16e62ea
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
to bridge system properties / env vars to declarative config
Relates #14192
Alternative implementation for #14767