[dynamic control] Move away from JSON requirement#2652
[dynamic control] Move away from JSON requirement#2652jackshirazi wants to merge 3 commits intoopen-telemetry:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request refactors the telemetry policy implementation to move away from JSON-based specifications to a type-specific subclass approach, aligning with the updated Telemetry Policy OTEP. The change replaces the generic TelemetryPolicy class that previously held a JsonNode spec field with a cleaner architecture where specific policy types extend TelemetryPolicy.
Changes:
- Removes JSON dependency from
TelemetryPolicycore class, eliminating thespecfield - Introduces
TraceSamplingRatePolicyas a concrete subclass with strong typing - Updates validation and implementation logic to work with typed policies instead of JSON structures
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| TelemetryPolicy.java | Removes JsonNode spec field and getSpec() method; simplifies to type-only base class |
| TraceSamplingRatePolicy.java | New concrete subclass holding probability value with constructor validation |
| TraceSamplingValidator.java | Updated to return TraceSamplingRatePolicy instances instead of generic policies with JSON specs |
| TraceSamplingRatePolicyImplementer.java | Refactored to use instanceof checks and direct probability access instead of JSON parsing |
| TraceSamplingValidatorTest.java | Updated assertions to check for TraceSamplingRatePolicy instances and use getProbability() |
| TraceSamplingRatePolicyImplementerTest.java | Replaced JSON-based test helpers with direct TraceSamplingRatePolicy instantiation |
| LinePerPolicyFileProviderTest.java | Updated mock validator to use single-argument TelemetryPolicy constructor |
...c-control/src/main/java/io/opentelemetry/contrib/dynamic/policy/TraceSamplingRatePolicy.java
Show resolved
Hide resolved
dynamic-control/src/main/java/io/opentelemetry/contrib/dynamic/policy/TelemetryPolicy.java
Outdated
Show resolved
Hide resolved
...c-control/src/main/java/io/opentelemetry/contrib/dynamic/policy/TraceSamplingRatePolicy.java
Show resolved
Hide resolved
...c-control/src/main/java/io/opentelemetry/contrib/dynamic/policy/TraceSamplingRatePolicy.java
Show resolved
Hide resolved
dynamic-control/src/main/java/io/opentelemetry/contrib/dynamic/policy/TelemetryPolicy.java
Show resolved
Hide resolved
...est/java/io/opentelemetry/contrib/dynamic/policy/TraceSamplingRatePolicyImplementerTest.java
Outdated
Show resolved
Hide resolved
...est/java/io/opentelemetry/contrib/dynamic/policy/TraceSamplingRatePolicyImplementerTest.java
Show resolved
Hide resolved
...rc/main/java/io/opentelemetry/contrib/dynamic/policy/TraceSamplingRatePolicyImplementer.java
Show resolved
Hide resolved
dynamic-control/src/main/java/io/opentelemetry/contrib/dynamic/policy/TelemetryPolicy.java
Show resolved
Hide resolved
| * | ||
| * @see io.opentelemetry.contrib.dynamic.policy | ||
| */ | ||
| public class TelemetryPolicy { |
There was a problem hiding this comment.
Not a blocker, but I'm curious if it makes sense to turn this into an interface to make it more flexible/easier to extend? I don't have a specific use case in mind where more flexibility would be needed; it's just that the rules around dynamic support are changing so often that it might be useful to have such flexibility.
There was a problem hiding this comment.
for now its useful as a concrete class for resetting to default. I've considered having it abstract or as an interface but it seems most useful at the moment as a concrete class. My aim is to have a working extension that supports sampling rate changes, then add further functionality and see how it changes as needed. Since this is all experimental, letting it evolve as needed rather than trying to anticipate the end state, is the preferred way to go (I think)
Description:
The Telemetry Policy OTEP has moved away from a specific JSON requirement to a non-specific data structure. First step in moving to this updated spec is to migrate away from JSON in the telemetry policy. As a by product, it's cleaner in many ways
Existing Issue(s):
#2546
Testing:
Tests updated
Documentation:
Documentation has been changed accordingly but the module documentation is the same as it's the same architecture
Outstanding items:
Although this is cleaner, it means there will be an additional mapping infrastructure needed to map custom structures into providers