Set attribute on exemplar spans #3723
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -993,6 +993,10 @@ 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`. | ||||||||||||||
|
|
||||||||||||||
| If a span is to be used as an exemplar, the ExemplarReservoir MUST set the | ||||||||||||||
| span attribute `"otel.exemplar"` to the value `true`, otherwise it MUST | ||||||||||||||
| not change the attribute. | ||||||||||||||
|
|
||||||||||||||
| 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 | ||||||||||||||
|
|
@@ -1028,7 +1032,7 @@ algorithm](https://en.wikipedia.org/wiki/Reservoir_sampling) can be used: | |||||||||||||
|
|
||||||||||||||
| ``` | ||||||||||||||
| bucket = random_integer(0, num_measurements_seen) | ||||||||||||||
| if bucket < num_buckets && reservoir[bucket] == null then | ||||||||||||||
|
There was a problem hiding this comment. This will drastically change the default sampling algorithm. It would no longer be a "uniformly-weighted sampling algorithm based on the number of samples the reservoir has seen so far to determine if the offered measurements should be sampled." I don't think this is an appropriate change for all use cases. It seems like a good motivation for a custom reservoir to be supported instead. There was a problem hiding this comment. Very good point - I hadn't realised that. Being able to provide my own (or switch to a provided "tail-sampling friendly") reservoir would be great. Alternatively, we could keep the algorithm as-is but still add the There was a problem hiding this comment. Being able to provide my own (or switch to a provided "tail-sampling friendly") reservoir would be great. That is already allowed in the spec! There was a problem hiding this comment.
I'll go give it a try! I've updated the PR to undo the changes to the fixed-size reservoir. I think this approach would be better long-term, even if it means keeping more spans than is needed. We could add some disclaimer explaining the trade-off between this and the histogram reservoir in terms of tail-sampling? There was a problem hiding this comment. I've changed the new rule in the spec to align with this approach. A little more maths suggests the ratio between the number of exemplars (
I'd personally be fine keeping that many spans unnecessarily. If it became a problem I'd switch to the histogram-based reservoir. I've updated the java PR with this approach. There was a problem hiding this comment. This is probably contradicting the "uniformely-weigted sampling algorithm based on the number of samples seen so far", as once the buckets are filled, all subsequent measurements will be never be stored into reservoir.! (nit perf, probably an implementation optizmiation: the cpu cycles spent on generating random is not required as well once the buckets are filled)... There was a problem hiding this comment. Just saw this is already called out in #3723 (comment) |
||||||||||||||
| reservoir[bucket] = measurement | ||||||||||||||
| end | ||||||||||||||
| ``` | ||||||||||||||
|
|
@@ -1044,13 +1048,13 @@ SHOULD be used. | |||||||||||||
| #### AlignedHistogramBucketExemplarReservoir | ||||||||||||||
|
|
||||||||||||||
| This Exemplar reservoir MUST take a configuration parameter that is the | ||||||||||||||
| configuration of a Histogram. This implementation MUST keep the first seen | ||||||||||||||
|
There was a problem hiding this comment. This reservoir is designed to match Prometheus behavior of today. Rather than changing this specification, I'd prefer instead to expose a mechanism for users to specify their own custom Exemplar Reservoirs. This is the right way going forward to allow users to better sample the exemplars for their own use case or do more intelligent per-application sampling (similarly to how we expose Samplers for tracing). |
||||||||||||||
| measurement that falls within a histogram bucket. The reservoir will accept | ||||||||||||||
| measurements using the equivalent of the following naive algorithm: | ||||||||||||||
|
|
||||||||||||||
| ``` | ||||||||||||||
| bucket = find_histogram_bucket(measurement) | ||||||||||||||
| if bucket < num_buckets && reservoir[bucket] == null then | ||||||||||||||
| reservoir[bucket] = measurement | ||||||||||||||
| end | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
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.
This will only work if we have a static set of exemplars. If an exemplar is sampled and later it is dropped from the reservoir due to other exemplars being sampled (simple-fixed-size-reservoir) there isn't a way to return the span to its original state currently because there is no API to drop an attribute.
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 attribute could be changed to indicate that the span was considered/initially-stored in an ExemplarReservoir, and the can be used by tail-sampler as an additional input when making sampling decision by prioritizing spans with this attribute. Not perfect, but still an improvement.