Finish wiring exemplars into metrics SDK #3599
Conversation
- Add an implementation of naive fixed-bucket reservoir sampling - Add an implementation of FixedSizeHistogram reservoir sampling where latest-measurement per-histogram-bucket is kept. - Add assertions for exemplars.
- Create new random holder abstraction that allows - ThreadLocalRandom when efficient - simple detection of Android + workaround for TLR - Overridable with mocked random for testing. - Update tracing RandomIdGenerator to leverage new RnadomHolder - Expand exemplar reservoir tests to use new random holder - Remove android-only random id generator test
…r-implementations
- Expose classes for aggregation configuration that are visible in exemplar sampler - Create default exemplar sampler that samples w/ traces - Expand metric benchmarks to include exemplar + no-exemplar to check overhead.
jsuereth
force-pushed
the
wip-exemplar-complete
branch
from
a0126d7 to
abb302d
Compare
- Expose classes for aggregation configuration that are visible in exemplar sampler - Create default exemplar sampler that samples w/ traces - Expand metric benchmarks to include exemplar + no-exemplar to check overhead.
jsuereth
force-pushed
the
wip-exemplar-complete
branch
from
0f79ce9 to
a2f22bb
Compare
…rating off AggregatorFactory -> Aggregation.
…pentelemetry-java into wip-exemplar-complete
Codecov Report
@@ Coverage Diff @@
## main #3599 +/- ##
============================================
+ Coverage 87.63% 88.94% +1.30%
- Complexity 3678 3691 +13
============================================
Files 438 442 +4
Lines 11707 11562 -145
Branches 1128 1113 -15
============================================
+ Hits 10260 10284 +24
+ Misses 1078 900 -178
- Partials 369 378 +9
Continue to review full report at Codecov.
|
jsuereth
requested review from
anuraaga,
arminru,
bogdandrutu,
carlosalberto and
jkwatson
as code owners
|
|
||
| | System property | Environment variable | Description | | ||
| |--------------------------|--------------------------|-----------------------------------------------------------------------------------| | ||
| | otel.metrics.exemplar.filter | OTEL_METRICS_EXEMPLAR_FILTER | The filter for exemplar sampling. Can be `NONE`, `ALL` or `WITH_SAMPLED_TRACE`. Default is `WITH_SAMPLED_TRACE`.| |
There was a problem hiding this comment.
Not for this PR since we're correctly reflecting the spec, but all of our other enum-type SDK config uses lowercase, not uppercase, would be nice if it can be fixed @jsuereth I don't know if we have time before the release though
There was a problem hiding this comment.
This is a reasonable thing to fix. If all other spec uses lower case, that's an easy bugfix. We're only in feature-freeze right now.
There was a problem hiding this comment.
If it's ok, I'll fix this in a follow up PR once this is submitted.
| @@ -23,4 +23,6 @@ dependencies { | |||
| testImplementation(project(":sdk:metrics-testing")) | |||
| testImplementation(project(":sdk:testing")) | |||
| testImplementation("com.google.guava:guava") | |||
|
|
|||
| jmh(project(":sdk:trace")) | |||
There was a problem hiding this comment.
I think it'd be better to put the benchmarks in sdk:all if possible this looks like an unexpected coupling of the SDK projects
There was a problem hiding this comment.
JMH for trace also uses metrics sdk right now: https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk/trace/build.gradle.kts#L37
This is in-line with our status-quo, but if you want to change that, I'd suggest outside this PR and we do it consistently.
| @@ -42,9 +42,9 @@ | |||
| private final MeterProviderSharedState sharedState; | |||
|
|
|||
| SdkMeterProvider( | |||
| Clock clock, Resource resource, ViewRegistry viewRegistry, ExemplarSampler exemplarSampler) { | |||
| Clock clock, Resource resource, ViewRegistry viewRegistry, ExemplarFilter exemplarFilter) { | |||
There was a problem hiding this comment.
Cool, I like Filter a lot better since we apply reservoir sampling after it anyways. Perhaps it allows renaming what we have as ExemplarReservoir to ExemplarFilter :-D
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/MeterSharedState.java
Outdated
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/view/Aggregation.java
Outdated
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/view/Aggregation.java
Outdated
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/view/Aggregation.java
Outdated
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/view/Aggregation.java
Outdated
Show resolved
Hide resolved
| @@ -99,8 +136,8 @@ public static Aggregation explictBucketHistogram() { | |||
| * | |||
| * @param temporality Whether to report DELTA or CUMULATIVE metrics. | |||
| */ | |||
| public static Aggregation explictBucketHistogram(AggregationTemporality temporality) { | |||
| return explictBucketHistogram( | |||
| public static Aggregation explicitBucketHistogram(AggregationTemporality temporality) { | |||
There was a problem hiding this comment.
do we need all of these options out of the box for v1? Could we limit the number of options we expose a bit more to start with? I see 4 methods for creating explicit bucket histograms which seems a tad excessive to start with.
eg. histogram() and explicitBucketHistogram() are the same thing. Seems like we don't necessarily need both of them.
There was a problem hiding this comment.
histogram is specified as "the best possible histogram" and is allowed to change for the exponential in the future.
Regarding explicit bucket, I was matching the optional parameters of the spec.
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/EmptyAggregator.java
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/MeterSharedState.java
Show resolved
Hide resolved
...rics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/SynchronousMetricStorage.java
Outdated
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/view/DefaultAggregation.java
Outdated
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/view/LastValueAggregation.java
Outdated
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/view/NoAggregation.java
Outdated
Show resolved
Hide resolved
...ics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/AsynchronousMetricStorage.java
Outdated
Show resolved
Hide resolved
Finishes wiring the public-facing API of Exemplar sampling into the SDK. After this PR exemplars should begin flowing through the metrics SDK, with configuration exposed to enable/disable. Not all desired customizations are exposed.
ExemplarSamplerinterface pending further spec discussion.ExemplarFilterconfiguration on the SDKExemplarReservoirdirectly to the aggregator.Follow-on (post SDK public facing API complete)
Current benchmark
API_ONLY = no sdk wired
SDK_NO_EXEMPLARS = neverSample() filter on exemplars
SDK = ALL measurements include a sampled trace (worst case scenario)
Optimisations included:
Follow up benchmark work:
Failed optimisations: