-
Notifications
You must be signed in to change notification settings - Fork 168
declarative config: support Inferred spans #2030
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
base: main
Are you sure you want to change the base?
Conversation
b119720 to
8d60820
Compare
|
@JonasKunz please have a look |
|
@jaydeluca please have a look |
...rc/test/java/io/opentelemetry/contrib/inferredspans/InferredSpansCustomizerProviderTest.java
Outdated
Show resolved
Hide resolved
...rc/test/java/io/opentelemetry/contrib/inferredspans/InferredSpansCustomizerProviderTest.java
Outdated
Show resolved
Hide resolved
|
@jaydeluca please check again |
This reverts commit 26ce159.
fe931a3 to
7b973c3
Compare
|
(testing out the copilot-instructions.md) |
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.
Pull Request Overview
This PR refactors the inferred spans configuration to support declarative YAML configuration and consolidates configuration logic. The changes enable the experimental_inferred_spans processor to be configured via OpenTelemetry's declarative configuration format, while maintaining backward compatibility with the existing autoconfiguration approach.
Key changes:
- Adds support for declarative YAML configuration through a new
InferredSpansSpanProcessorProvider - Extracts shared configuration logic from
InferredSpansAutoConfiginto a newInferredSpansConfigclass - Adds an
enabledfield toInferredSpansConfigurationto support conditional processor creation - Improves test isolation by using JUnit 5's
@TempDirfor temporary file management
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
InferredSpansSpanProcessorProvider.java |
New provider class that enables declarative configuration support for the inferred spans processor |
InferredSpansConfig.java |
Extracted configuration logic from InferredSpansAutoConfig to enable reuse across configuration approaches |
InferredSpansAutoConfig.java |
Refactored to delegate configuration logic to InferredSpansConfig |
InferredSpansProcessorBuilder.java |
Added profilerEnabled method to support the new enabled configuration field |
InferredSpansProcessor.java |
Updated constructor call to include new tempDir parameter |
SamplingProfiler.java |
Added tempDir field and early exit logic when profiling is disabled |
InferredSpansConfiguration.java |
Added enabled field and corresponding getter method |
SamplingProfilerTest.java |
Refactored to use @TempDir for better test isolation |
InferredSpansAutoConfigTest.java |
Updated test references from InferredSpansAutoConfig constants to InferredSpansConfig constants |
InferredSpansSpanProcessorProviderTest.java |
New test file for declarative configuration functionality |
build.gradle.kts |
Added dependencies required for declarative configuration support |
README.md |
Added documentation section explaining declarative configuration usage |
Comments suppressed due to low confidence (1)
inferred-spans/README.md:37
- Corrected spelling: 'he frequency' should be 'The frequency'.
| otel.inferred.spans.sampling.interval <br/> OTEL_INFERRED_SPANS_SAMPLING_INTERVAL | `50ms` | he frequency at which stack traces are gathered within a profiling session. The lower you set it, the more accurate the durations will be. This comes at the expense of higher overhead and more spans for potentially irrelevant operations. The minimal duration of a profiling-inferred span is the same as the value of this setting. |
| tracer_provider: | ||
| processors: | ||
| - experimental_inferred_spans: | ||
| enabled: true # true by default unlike autoconfiguration described above |
Copilot
AI
Oct 31, 2025
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.
The comment states 'true by default' for declarative configuration, but the code in InferredSpansSpanProcessorProvider.java line 42 shows properties.getBoolean(InferredSpansConfig.ENABLED_OPTION, true), which indeed defaults to true. However, this creates a confusing inconsistency with autoconfiguration which defaults to false. Consider clarifying this difference more prominently in the documentation, perhaps in a separate note or warning box, to prevent user confusion about the different default behaviors.
| tracer_provider: | ||
| processors: | ||
| - experimental_inferred_spans: | ||
| enabled: true # true by default unlike autoconfiguration described above |
Copilot
AI
Oct 31, 2025
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.
The YAML example should also include the enabled property in the list of available options. While it's shown in the example, the declarative configuration section doesn't mention that all autoconfiguration options (including enabled, logging_enabled, backup_diagnostic_files, etc.) are available. Consider adding a note or explicitly mentioning the full list of available properties.
The declarative config bridge has been copied from the agent - eventually, we can use only one, but that's a later step.
Fixes #2029