Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

POC: Set attribute on span if used as exemplar #5915

Closed
wants to merge 2 commits into from

Conversation

samwright
Copy link

Proof of Concept for a way to support metric exemplars and tail-based sampling at the same time. Hopefully this will spur discussion on how we want to support this use-case.

Relevant issue: open-telemetry/opentelemetry-specification#2922

This makes a few big changes:

  • HistogramExemplarReservoir and RandomFixedSizeExemplarReservoir now collect the first instead of the last exemplar sample they see. This lets us know whether or not a span will be used as an exemplar while it is still current.
  • ExemplarReservoir.offerXXXMeasurement now returns boolean so callers know whether or not the span is being used as an exemplar.
  • Suggest otel.exemplar as a potential new attribute that, when set to true means the sample is being used as an exemplar.

This approach allows tail-based samplers to ensure that all traces containing spans with otel.exemplar == true are kept, while the rest can be sampled without accidentally dropping a span used as an exemplar.

@codecov
Copy link

codecov bot commented Oct 14, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Files Coverage Δ
...k/metrics/internal/exemplar/ExemplarReservoir.java 100.00% <100.00%> (ø)
.../internal/exemplar/HistogramExemplarReservoir.java 90.00% <100.00%> (-0.91%) ⬇️
...trics/internal/exemplar/NoopExemplarReservoir.java 100.00% <100.00%> (ø)
...y/sdk/metrics/internal/exemplar/ReservoirCell.java 91.48% <100.00%> (+1.01%) ⬆️
...ternal/exemplar/SpanUpdatingExemplarReservoir.java 100.00% <100.00%> (ø)
...nal/view/Base2ExponentialHistogramAggregation.java 100.00% <100.00%> (ø)
...ernal/view/ExplicitBucketHistogramAggregation.java 100.00% <100.00%> (ø)
...etry/sdk/metrics/internal/view/SumAggregation.java 91.66% <100.00%> (+0.75%) ⬆️
.../internal/exemplar/FixedSizeExemplarReservoir.java 89.28% <66.66%> (ø)
...s/internal/exemplar/FilteredExemplarReservoir.java 66.66% <25.00%> (-15.16%) ⬇️

... and 1 file with indirect coverage changes

📢 Thoughts on this report? Let us know!.

@jack-berg
Copy link
Member

@samwright the exemplar spec is currently listed as feature freeze. This requires a change to the exemplar reservoir interface such that it returns boolean. Would you care to bring this up in the 10/17 spec SIG? The group meets on Tuesdays at 8am PST and has recently been discussing stabilizing the exemplar spec. Would be good to talk about this use case.

@samwright
Copy link
Author

Hi Jack, I'll bring that up at the SIG - sounds like fun! I'll add myself to the agenda. I've had another go at this problem without changing any interfaces here: #5922 . The changes in the spec should be a bit simpler, e.g.

If a span is to be used as an exemplar, it MUST be enriched with a span attribute otel.exemplar where the value is true.

and under AlignedHistogramBucketExemplarReservoir change from last -> first, i.e.

This implementation MUST keep the first seen measurement that falls within a histogram bucket.

@jack-berg
Copy link
Member

@samwright IIRC correctly, the conclusion was that its possible to build a custom reservoir which has the storage semantics needed such that you can guarantee that a span marked with otel.exemplar=true is recorded in the exemplar reservoir. If that's correct, then I think what we need in this repo is the ability to customize the exemplar reservoir using the view API, which is a different scope than this POC.

@jack-berg jack-berg closed this Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants