Prototype exposing the exemplar reservoir for upcoming spec changes #5960
Conversation
- Create ExemplarReservoirFactory to help abstract over Double/Long hell - Create AggregationExtension that allows specifying an exemplar reservoir factory - Add a test which hacks RNG to make sure this all works correctly.
Codecov ReportAttention:
... and 5 files with indirect coverage changes 📢 Thoughts on this report? Let us know!. |
| View.builder() | ||
| .setAggregation( | ||
| ((AggregationExtension) Aggregation.sum()) | ||
| .setExemplarReservoirFactory(reservoirFactory)) |
There was a problem hiding this comment.
Should exemplar reservoir be a configuration parameter that hangs off an aggregation or a top level view config option? I.e. View.builder.setExemplarReservoirFactory(...).setAggregation(..)
There was a problem hiding this comment.
This is one of the things we need to decide.
- Right now, ExemplarReservoir (defaults) are decided by aggregation.
- If a user wanted to leverage "default" aggregation, but different reservoir by-instrument type or aggregator choice, we're actually lacking an interplay between those two. E.g. trying to ensure Explicit buckets are the same between exemplars + histograms.
Personally, I think we could go either way here. I'm happy to extract it out further if we want.
There was a problem hiding this comment.
Another interesting side affect of having exemplar reservoir be a configurable property of Aggregation is that it causes the reservoir be something that MetricReader / MetricExporter have influence over, since MetricReader / MetricExporter can dictate the default Aggregation as a function of instrument kind.
There was a problem hiding this comment.
I actually really like that interaction. It allows exporters to optimise exemplar choice for the backend they speak to. Do you think we should encode this in the specification?
There was a problem hiding this comment.
I think it ought to be in the spec if we're going to allow that type of configuration in the java implementation.
Another thought: I think we probably should introduce an AggregationBuilder instead of allowing Aggregation instances to be mutated after instantiation with setAggregation.
I propose we add new method static AggregationBuilder Aggregation#builder() for accessing an AggregationBuilder, defined as follows:
public interface AggregationBuilder {
// Configure aggregation
AggregationBuilder setDrop();
AggregationBuilder setSum();
AggregationBuilder setLastValue();
AggregationBuilder setExplicitBucketHistogram();
AggregationBuilder setExplicitBucketHistogram(List<Double> bucketBoundaries);
AggregationBuilder setBase2ExponentialHistogram();
AggregationBuilder setBase2ExponentialHistogram(int maxBuckets, int maxScale);
// Configure reservoir
AggregationBuilder setExemplarReservoirFactory(Supplier<ExemplarReservoir<?>> factory);
Aggregation build();
}
Usage would be something like:
SdkMeterProvider.builder()
.registerView(
InstrumentSelector.builder().build(),
View.builder()
.setAggregation(Aggregation.builder()
.setSum()
.setExemplarReservoirFactory(ExemplarReservoirFactory.noSamples())
.build())
.build());
|
Just the one comment but conceptually I think this makes sense. |
|
Wanted to add another comment about things we should likely be exposing to users with this change (once specification is written and publicized):
|
|
|
||
| @Override | ||
| public String toString() { | ||
| return "fixedSize(" + size + ")"; |
There was a problem hiding this comment.
I'd like to see this split out into a class so we can provide a more conventional toString implementation:
| return "fixedSize(" + size + ")"; | |
| return "FixedSizeReservoirFactory(" + size + ")"; |
| this( | ||
| ExemplarReservoirFactory.fixedSize( | ||
| Clock.getDefault(), | ||
| Runtime.getRuntime().availableProcessors(), |
There was a problem hiding this comment.
Do we still think Runtime.getRuntime().availableProcessors() is best if the spec provides the new language:
the default size MAY be
the number of possible concurrent threads (e.g. numer of CPUs) to help reduce
contention. Otherwise, a default size of1SHOULD be used.
There was a problem hiding this comment.
I wrote the specification with this bit of code in mind.... What would you instead like to see?
The rationale here was a measurable difference in contention.
| return new ExplicitBucketHistogramAggregation(bucketBoundaries); | ||
| return new ExplicitBucketHistogramAggregation( | ||
| bucketBoundaries, | ||
| ExemplarReservoirFactory.histogramBucket(Clock.getDefault(), bucketBoundaries)); |
There was a problem hiding this comment.
I realized that we need to use fixed size by default if bucketBoundaries.size() == 0.
| View.builder() | ||
| .setAggregation( | ||
| ((AggregationExtension) Aggregation.sum()) | ||
| .setExemplarReservoirFactory(reservoirFactory)) |
There was a problem hiding this comment.
I think it ought to be in the spec if we're going to allow that type of configuration in the java implementation.
Another thought: I think we probably should introduce an AggregationBuilder instead of allowing Aggregation instances to be mutated after instantiation with setAggregation.
I propose we add new method static AggregationBuilder Aggregation#builder() for accessing an AggregationBuilder, defined as follows:
public interface AggregationBuilder {
// Configure aggregation
AggregationBuilder setDrop();
AggregationBuilder setSum();
AggregationBuilder setLastValue();
AggregationBuilder setExplicitBucketHistogram();
AggregationBuilder setExplicitBucketHistogram(List<Double> bucketBoundaries);
AggregationBuilder setBase2ExponentialHistogram();
AggregationBuilder setBase2ExponentialHistogram(int maxBuckets, int maxScale);
// Configure reservoir
AggregationBuilder setExemplarReservoirFactory(Supplier<ExemplarReservoir<?>> factory);
Aggregation build();
}
Usage would be something like:
SdkMeterProvider.builder()
.registerView(
InstrumentSelector.builder().build(),
View.builder()
.setAggregation(Aggregation.builder()
.setSum()
.setExemplarReservoirFactory(ExemplarReservoirFactory.noSamples())
.build())
.build());
Fixes #2922 Fixes #3812 Related to #3756 ## Changes - Cleans up language around exposing `ExemplarReservoir`. (Remove TODO, e.g.) - Remove `ExemplarFilter` interface but keep configuration options. (see #3812) - Clarify / simplify Spec Compliance matrix for existing state of the Exemplar Specification. Prototype in java: open-telemetry/opentelemetry-java#5960
This is for open discussion on how we want to expose customization of ExemplarReservoir to users. One of the two remaining blockers for stabilizing Exemplar specification.
cc @jack-berg