From c7a9a81b8a3fcf507e7072f6ef36a5cde6ff829e Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Fri, 10 Nov 2023 14:49:16 -0500 Subject: [PATCH 01/12] Fixes #2205 - Update example algorithm to account for initial reservoir fill. --- specification/metrics/sdk.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/specification/metrics/sdk.md b/specification/metrics/sdk.md index 0ac9ad9f29e..02d9fee1aa9 100644 --- a/specification/metrics/sdk.md +++ b/specification/metrics/sdk.md @@ -1027,7 +1027,11 @@ measurements should be sampled. For example, the [simple reservoir sampling algorithm](https://en.wikipedia.org/wiki/Reservoir_sampling) can be used: ``` - bucket = random_integer(0, num_measurements_seen) + if num_measurements_seen < num_buckets then + bucket = num_measurements_seen + else + bucket = random_integer(0, num_measurements_seen) + end if bucket < num_buckets then reservoir[bucket] = measurement end From c8c1baf441d1f71fa55da76a35dea2ccb12372d6 Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Fri, 10 Nov 2023 15:02:30 -0500 Subject: [PATCH 02/12] Update the default exemplar reservoirs. - Update fixed-size defaults to account for memory contention/optimisation. - Fix #3674: Set a default for exponential histograms. --- specification/metrics/sdk.md | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/specification/metrics/sdk.md b/specification/metrics/sdk.md index 02d9fee1aa9..03d083ae402 100644 --- a/specification/metrics/sdk.md +++ b/specification/metrics/sdk.md @@ -1015,9 +1015,15 @@ The SDK SHOULD include two types of built-in exemplar reservoirs: 1. `SimpleFixedSizeExemplarReservoir` 2. `AlignedHistogramBucketExemplarReservoir` -By default, explicit bucket histogram aggregation with more than 1 bucket will -use `AlignedHistogramBucketExemplarReservoir`. All other aggregations will use -`SimpleFixedSizeExemplarReservoir`. +By default, + +- Explicit bucket histogram aggregation with more than 1 bucket will +use `AlignedHistogramBucketExemplarReservoir`. +- Base2 Exponential Histogram Aggregation SHOULD use a + `SimpleFixedSizeExemplarReservoir` with a reservoir equal to the + smaller of the maximum number of buckets configured on the aggregation or + twenty (e.g. `min(20, max_buckets)`). +- All other aggregations will use `SimpleFixedSizeExemplarReservoir`. #### SimpleFixedSizeExemplarReservoir @@ -1042,8 +1048,9 @@ cycle. For the above example, that would mean that the `num_measurements_seen` count is reset every time the reservoir is collected. This Exemplar reservoir MAY take a configuration parameter for the size of the -reservoir pool. If no size configuration is provided, the default size of `1` -SHOULD be used. +reservoir pool. If no size configuration is provided, the default size MAY be +the number of possible concurrent threads (e.g. numer of CPUs) to help reduce +contention. Otherwise, a default size of `1` SHOULD be used. #### AlignedHistogramBucketExemplarReservoir From 431a7e3ead68fcb764fb6a5779c0bc7ddbaa0c66 Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Fri, 10 Nov 2023 15:47:57 -0500 Subject: [PATCH 03/12] Clarify that ExemplarFilter should be configred on MeterProvider. Partially fixes: #2421 --- specification/metrics/sdk.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/specification/metrics/sdk.md b/specification/metrics/sdk.md index 03d083ae402..ec28a27b665 100644 --- a/specification/metrics/sdk.md +++ b/specification/metrics/sdk.md @@ -972,6 +972,12 @@ Using this ExemplarFilter is as good as disabling Exemplar feature. An ExemplarFilter which makes those measurements eligible for being an Exemplar, which are recorded in the context of a sampled parent span. +#### Configuration + +The ExemplarFilter SHOULD be a configuration parameter of a `MeterProvider` for +an SDK. The default value for the filter SHOULD follow the +[environment variable specification](../configuration/sdk-environment-variables.md#exemplar). + ### ExemplarReservoir The `ExemplarReservoir` interface MUST provide a method to offer measurements From 6dc667c5c95761cfdca6dff3a0d5f12bb6004ff6 Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Fri, 10 Nov 2023 15:58:54 -0500 Subject: [PATCH 04/12] Improve clarity + flexibility on Exemplar Reservoirs. - Make it clear that one reservoir is created PER timeseries point. - Allow flexibility in Reservoir definition, based on feedback from Go SIG - #3669 --- specification/metrics/sdk.md | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/specification/metrics/sdk.md b/specification/metrics/sdk.md index ec28a27b665..648380a9d26 100644 --- a/specification/metrics/sdk.md +++ b/specification/metrics/sdk.md @@ -983,6 +983,10 @@ an SDK. The default value for the filter SHOULD follow the The `ExemplarReservoir` interface MUST provide a method to offer measurements to the reservoir and another to collect accumulated Exemplars. +A new `ExemplarReservoir` MUST be created for every known timeseries data point, +as determined by aggregation and view configuration. This data point, and its +set of defining attributes, are referred to as the associated timeseries point. + The "offer" method SHOULD accept measurements, including: - The `value` of the measurement. @@ -999,18 +1003,25 @@ span context and baggage can be inspected at this point. The "offer" method does not need to store all measurements it is given and MAY further sample beyond the `ExemplarFilter`. +The "offer" method MAY accept a filtered subset of `Attributes` which diverge +from the timeseries the reservoir is associated with. This MUST be clearly +documented in the API interface and the reservoir MUST be given the `Attributes` +associated with its timeseries point either at construction or in the "collect" +method. SDK authors are encouraged to benchmark whether this option works best +for their implementation. + The "collect" method MUST return accumulated `Exemplar`s. Exemplars are expected to abide by the `AggregationTemporality` of any metric point they are recorded with. In other words, Exemplars reported against a metric data point SHOULD have -occurred within the start/stop timestamps of that point. SDKs are free to +occurred within the start/stop timestamps of that point. SDKs are free to decide whether "collect" should also reset internal storage for delta temporal aggregation collection, or use a more optimal implementation. `Exemplar`s MUST retain any attributes available in the measurement that -are not preserved by aggregation or view configuration. Specifically, at a -minimum, joining together attributes on an `Exemplar` with those available -on its associated metric data point should result in the full set of attributes -from the original sample measurement. +are not preserved by aggregation or view configuration for the associated +timeseries. Specifically, at a minimum, joining together attributes on an +`Exemplar` with those available on its associated metric data point should +result in the full set of attributes from the original sample measurement. The `ExemplarReservoir` SHOULD avoid allocations when sampling exemplars. From b8644c6a80dde5b1b7bf52a58dc62bfeeda64eec Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Fri, 10 Nov 2023 16:08:50 -0500 Subject: [PATCH 05/12] Fix markdown errors. --- specification/metrics/sdk.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/specification/metrics/sdk.md b/specification/metrics/sdk.md index 648380a9d26..05750ab914c 100644 --- a/specification/metrics/sdk.md +++ b/specification/metrics/sdk.md @@ -54,6 +54,7 @@ linkTitle: SDK + [AlwaysOn](#alwayson) + [AlwaysOff](#alwaysoff) + [TraceBased](#tracebased) + + [Configuration](#configuration-2) * [ExemplarReservoir](#exemplarreservoir) * [Exemplar defaults](#exemplar-defaults) + [SimpleFixedSizeExemplarReservoir](#simplefixedsizeexemplarreservoir) @@ -1032,7 +1033,7 @@ The SDK SHOULD include two types of built-in exemplar reservoirs: 1. `SimpleFixedSizeExemplarReservoir` 2. `AlignedHistogramBucketExemplarReservoir` -By default, +By default: - Explicit bucket histogram aggregation with more than 1 bucket will use `AlignedHistogramBucketExemplarReservoir`. From ce97e3acdddcb78e436b679c72cbd21e5d23ac57 Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Fri, 10 Nov 2023 16:13:15 -0500 Subject: [PATCH 06/12] Added changelog. --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5debda72f5f..a42c7d8b8df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,9 @@ release. ### Metrics +- Clarifications and flexibility in Exemplar speicification. + ([#3760](https://github.com/open-telemetry/opentelemetry-specification/pull/3760)) + ### Logs ### Resource From c13e8ab9da4fb028b228a844a4fd9fc803607abb Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Wed, 15 Nov 2023 10:44:47 -0500 Subject: [PATCH 07/12] Fix wording to be more clear --- specification/metrics/sdk.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/specification/metrics/sdk.md b/specification/metrics/sdk.md index 05750ab914c..7d19cb75ea6 100644 --- a/specification/metrics/sdk.md +++ b/specification/metrics/sdk.md @@ -1020,9 +1020,9 @@ aggregation collection, or use a more optimal implementation. `Exemplar`s MUST retain any attributes available in the measurement that are not preserved by aggregation or view configuration for the associated -timeseries. Specifically, at a minimum, joining together attributes on an -`Exemplar` with those available on its associated metric data point should -result in the full set of attributes from the original sample measurement. +timeseries. Joining together attributes on an `Exemplar` with +those available on its associated metric data point should result in the +full set of attributes from the original sample measurement. The `ExemplarReservoir` SHOULD avoid allocations when sampling exemplars. From 2c368f79d5bff3fede03955cd57a61c8ab5e1891 Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Fri, 17 Nov 2023 12:35:17 -0500 Subject: [PATCH 08/12] remove the term pool to avoid confusion. --- specification/metrics/sdk.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/specification/metrics/sdk.md b/specification/metrics/sdk.md index 7d19cb75ea6..bb649db453f 100644 --- a/specification/metrics/sdk.md +++ b/specification/metrics/sdk.md @@ -1066,7 +1066,7 @@ cycle. For the above example, that would mean that the `num_measurements_seen` count is reset every time the reservoir is collected. This Exemplar reservoir MAY take a configuration parameter for the size of the -reservoir pool. If no size configuration is provided, the default size MAY be +reservoir. If no size configuration is provided, the default size MAY be the number of possible concurrent threads (e.g. numer of CPUs) to help reduce contention. Otherwise, a default size of `1` SHOULD be used. From f03afe52af12f66ffe9f916ae707f22e181c295c Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Fri, 17 Nov 2023 12:46:56 -0500 Subject: [PATCH 09/12] Attempted clarification. --- specification/metrics/sdk.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/specification/metrics/sdk.md b/specification/metrics/sdk.md index bb649db453f..91380177a9f 100644 --- a/specification/metrics/sdk.md +++ b/specification/metrics/sdk.md @@ -1007,7 +1007,8 @@ MAY further sample beyond the `ExemplarFilter`. The "offer" method MAY accept a filtered subset of `Attributes` which diverge from the timeseries the reservoir is associated with. This MUST be clearly documented in the API interface and the reservoir MUST be given the `Attributes` -associated with its timeseries point either at construction or in the "collect" +associated with its timeseries point either at construction so that additonal +sampling performed by the reservoir has access to all attributes in the "offer" method. SDK authors are encouraged to benchmark whether this option works best for their implementation. From d2a293ea0d60c586170104704d11327a8c35a470 Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Fri, 17 Nov 2023 12:48:43 -0500 Subject: [PATCH 10/12] one more rewording. --- specification/metrics/sdk.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/specification/metrics/sdk.md b/specification/metrics/sdk.md index 91380177a9f..230fda64114 100644 --- a/specification/metrics/sdk.md +++ b/specification/metrics/sdk.md @@ -1008,9 +1008,9 @@ The "offer" method MAY accept a filtered subset of `Attributes` which diverge from the timeseries the reservoir is associated with. This MUST be clearly documented in the API interface and the reservoir MUST be given the `Attributes` associated with its timeseries point either at construction so that additonal -sampling performed by the reservoir has access to all attributes in the "offer" -method. SDK authors are encouraged to benchmark whether this option works best -for their implementation. +sampling performed by the reservoir has access to all attributes from a +measurement in the "offer" method. SDK authors are encouraged to benchmark +whether this option works best for their implementation. The "collect" method MUST return accumulated `Exemplar`s. Exemplars are expected to abide by the `AggregationTemporality` of any metric point they are recorded From ffc1dbf7191f440a8f829b8e1bc589468315d2f4 Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Thu, 30 Nov 2023 15:42:36 -0500 Subject: [PATCH 11/12] Update specification/metrics/sdk.md Co-authored-by: David Ashpole --- specification/metrics/sdk.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/specification/metrics/sdk.md b/specification/metrics/sdk.md index 230fda64114..3482ca11379 100644 --- a/specification/metrics/sdk.md +++ b/specification/metrics/sdk.md @@ -1007,7 +1007,7 @@ MAY further sample beyond the `ExemplarFilter`. The "offer" method MAY accept a filtered subset of `Attributes` which diverge from the timeseries the reservoir is associated with. This MUST be clearly documented in the API interface and the reservoir MUST be given the `Attributes` -associated with its timeseries point either at construction so that additonal +associated with its timeseries point either at construction so that additional sampling performed by the reservoir has access to all attributes from a measurement in the "offer" method. SDK authors are encouraged to benchmark whether this option works best for their implementation. From 1c339679f2003c8fc0137a4641d8f3c33dd28e63 Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Fri, 1 Dec 2023 11:44:11 -0500 Subject: [PATCH 12/12] Declare default for filter in the spec. --- specification/metrics/sdk.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/specification/metrics/sdk.md b/specification/metrics/sdk.md index 3482ca11379..ee62c5a4557 100644 --- a/specification/metrics/sdk.md +++ b/specification/metrics/sdk.md @@ -976,8 +976,8 @@ Exemplar, which are recorded in the context of a sampled parent span. #### Configuration The ExemplarFilter SHOULD be a configuration parameter of a `MeterProvider` for -an SDK. The default value for the filter SHOULD follow the -[environment variable specification](../configuration/sdk-environment-variables.md#exemplar). +an SDK. The default value SHOULD be `TraceBased`. The filter configuration +SHOULD follow the [environment variable specification](../configuration/sdk-environment-variables.md#exemplar). ### ExemplarReservoir