detect inferred spans on tracer name#1097
Conversation
📝 WalkthroughWalkthroughThis PR refactors inferred span detection from checking a boolean span attribute to using the instrumentation scope name. A new 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| testImplementation("io.opentelemetry:opentelemetry-api-incubator") | ||
|
|
||
| // only to get reference to upstream inferred tracer name | ||
| testCompileOnly(libs.contribInferredSpans); |
There was a problem hiding this comment.
for reviewer: only this line is added, other changes are just reformatting.
|
|
||
| import io.opentelemetry.sdk.trace.ReadableSpan; | ||
|
|
||
| public class InferredSpanDetector { |
There was a problem hiding this comment.
do we really need a separated class just for this static method?
There was a problem hiding this comment.
I haven't found any other utility class with static methods where to put this. Do you think it would be better to rename the class to SpanUtil to suggest that it may be extended with other span-related code in the future ?
jackshirazi
left a comment
jackshirazi
left a comment
There was a problem hiding this comment.
approved, but note comment
…rred-spans_remove-attribute
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@testing/integration-tests/inferred-spans-test/src/test/java/InferredSpansTest.java`:
- Around line 47-50: The test currently builds inferredSpansScope with
InstrumentationScopeInfo.builder("inferred-spans").setVersion(System.getProperty("test.contrib-inferred-spans.version"))
which can pass a null version when the system property is missing; change this
to read the property into a local variable, validate it is non-null/non-empty
(fail fast with a clear AssertionError or IllegalStateException mentioning
"test.contrib-inferred-spans.version" and the test name InferredSpansTest), and
only call setVersion(...) when the value is present so the test fails
immediately with a clear message instead of producing a scope with a null
version that causes confusing assertion mismatches.
- Around line 47-50: The InstrumentationScopeInfo builder is being given a
potentially null version via
System.getProperty("test.contrib-inferred-spans.version"), causing assertion
mismatches; change the inferredSpansScope construction to supply a non-null
default (e.g., use
java.util.Objects.requireNonNullElse(System.getProperty("test.contrib-inferred-spans.version"),
"") or similar) when calling setVersion so the builder never receives null, and
add the import for java.util.Objects; keep the rest of the
InstrumentationScopeInfo.builder("inferred-spans") call unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 8bb406e8-7a77-478d-a15a-a9f6909304d0
📒 Files selected for processing (11)
common/build.gradle.ktscommon/src/main/java/co/elastic/otel/common/ElasticAttributes.javacommon/src/main/java/co/elastic/otel/common/LocalRootSpan.javacommon/src/main/java/co/elastic/otel/common/SpanUtils.javacommon/src/test/java/co/elastic/otel/common/LocalRootSpanTest.javacommon/src/test/java/co/elastic/otel/common/SpanUtilsTest.javacustom/src/main/java/co/elastic/otel/SpanStackTraceFilter.javacustom/src/test/java/co/elastic/otel/SpanStackTraceFilterTest.javacustom/src/test/java/co/elastic/otel/SpanStackTraceTest.javatesting/integration-tests/inferred-spans-test/build.gradle.ktstesting/integration-tests/inferred-spans-test/src/test/java/InferredSpansTest.java
💤 Files with no reviewable changes (1)
- common/src/main/java/co/elastic/otel/common/ElasticAttributes.java
| private static final InstrumentationScopeInfo inferredSpansScope = | ||
| InstrumentationScopeInfo.builder("inferred-spans") | ||
| .setVersion(System.getProperty("test.contrib-inferred-spans.version")) | ||
| .build(); |
There was a problem hiding this comment.
setVersion(null) silently degrades the scope assertion when run outside Gradle.
If test.contrib-inferred-spans.version is absent (e.g., IDE run), System.getProperty(...) returns null, producing a scope with a null version. The hasInstrumentationScopeInfo assertion on line 95 will then mismatch against the real span's non-null version — the test fails with no clear error about the missing property.
🛡️ Proposed fix: fail fast with a clear message
+import java.util.Objects;
...
- private static final InstrumentationScopeInfo inferredSpansScope =
- InstrumentationScopeInfo.builder("inferred-spans")
- .setVersion(System.getProperty("test.contrib-inferred-spans.version"))
- .build();
+ private static final InstrumentationScopeInfo inferredSpansScope =
+ InstrumentationScopeInfo.builder("inferred-spans")
+ .setVersion(
+ Objects.requireNonNull(
+ System.getProperty("test.contrib-inferred-spans.version"),
+ "System property 'test.contrib-inferred-spans.version' must be set"))
+ .build();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static final InstrumentationScopeInfo inferredSpansScope = | |
| InstrumentationScopeInfo.builder("inferred-spans") | |
| .setVersion(System.getProperty("test.contrib-inferred-spans.version")) | |
| .build(); | |
| private static final InstrumentationScopeInfo inferredSpansScope = | |
| InstrumentationScopeInfo.builder("inferred-spans") | |
| .setVersion( | |
| Objects.requireNonNull( | |
| System.getProperty("test.contrib-inferred-spans.version"), | |
| "System property 'test.contrib-inferred-spans.version' must be set")) | |
| .build(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@testing/integration-tests/inferred-spans-test/src/test/java/InferredSpansTest.java`
around lines 47 - 50, The test currently builds inferredSpansScope with
InstrumentationScopeInfo.builder("inferred-spans").setVersion(System.getProperty("test.contrib-inferred-spans.version"))
which can pass a null version when the system property is missing; change this
to read the property into a local variable, validate it is non-null/non-empty
(fail fast with a clear AssertionError or IllegalStateException mentioning
"test.contrib-inferred-spans.version" and the test name InferredSpansTest), and
only call setVersion(...) when the value is present so the test fails
immediately with a clear message instead of producing a scope with a null
version that causes confusing assertion mismatches.
setVersion(null) will cause assertion mismatches when the system property is absent.
If test.contrib-inferred-spans.version is not set (e.g., running the test directly from an IDE or a misconfigured CI job), System.getProperty(...) returns null, so inferredSpansScope is built with a null version. The hasInstrumentationScopeInfo assertion on line 95 then compares against a scope whose version is null, which won't match the real span's non-null scope version — silently failing the test in every non-Gradle environment.
A null-safe guard keeps the intent clear:
🛡️ Proposed fix
- private static final InstrumentationScopeInfo inferredSpansScope =
- InstrumentationScopeInfo.builder("inferred-spans")
- .setVersion(System.getProperty("test.contrib-inferred-spans.version"))
- .build();
+ private static final String INFERRED_SPANS_VERSION =
+ Objects.requireNonNull(
+ System.getProperty("test.contrib-inferred-spans.version"),
+ "System property 'test.contrib-inferred-spans.version' must be set");
+
+ private static final InstrumentationScopeInfo inferredSpansScope =
+ InstrumentationScopeInfo.builder("inferred-spans")
+ .setVersion(INFERRED_SPANS_VERSION)
+ .build();Also add import java.util.Objects; to the imports.
Does io.opentelemetry.sdk.common.InstrumentationScopeInfo.Builder.setVersion(null) throw NullPointerException or silently store null?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@testing/integration-tests/inferred-spans-test/src/test/java/InferredSpansTest.java`
around lines 47 - 50, The InstrumentationScopeInfo builder is being given a
potentially null version via
System.getProperty("test.contrib-inferred-spans.version"), causing assertion
mismatches; change the inferredSpansScope construction to supply a non-null
default (e.g., use
java.util.Objects.requireNonNullElse(System.getProperty("test.contrib-inferred-spans.version"),
"") or similar) when calling setVersion so the builder never receives null, and
add the import for java.util.Objects; keep the rest of the
InstrumentationScopeInfo.builder("inferred-spans") call unchanged.
2 participants
In open-telemetry/opentelemetry-java-contrib#2803 in contrib, we use the tracer name to filter the inferred spans.
In EDOT, we need to implement the same heuristic for consistency, also it allows to avoid relying on the internal, out of semconv and undocumented
is_inferredspan attribute.Instead, we rely on the tracer name which is expected to be stable and is equal to
inferred-spansfor inferred spans.This is mostly a refactor that anticipates that the
is_inferredspan attribute might change (or be removed) in the future, there is no visible change in behavior for end-users.