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

Metric Exemplars SDK Specification #1828

Merged
merged 14 commits into from
Aug 30, 2021
Merged

Conversation

jsuereth
Copy link
Contributor

Fixes #1797

Changes

Add Exemplar specification and customization to the metrics specification.

Related issues #1260 #617

Related oteps #113

Notes

  • This exposes two extensible interfaces. It's fine to drop these if they are contentious. The key important piece we must keep is the ability to turn OFF exemplars and the ability to limit exemplars to only measurements with associated sampled spans on context.
  • This diverges from the Exemplar OTEP in that it is less restrictive on behavior of the exemplar reservoir (currently). Additionally the aggregators available when the exemplar OTEP was written are not yet specified. We can refine this in a follow on PR as necessary. We should align any sampling decisions w/ the future of tracing sampling.
  • There are still some TODOs for discussion.

specification/metrics/sdk.md Show resolved Hide resolved
specification/metrics/sdk.md Show resolved Hide resolved
Copy link
Contributor

@jmacd jmacd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
@jmacd
Copy link
Contributor

jmacd commented Aug 12, 2021

FYI I have been working on trace sampling lately, and there is a lot of alignment between that work and this work. I have made claims that we can use weighted sampling to implement a hard rate limit on spans inside a tail sampler, and by extension I claim to know how to do the same for histogram exemplars. For a histogram aggregator outputting high-resolution histogram buckets you might not want one exemplar per bucket, and with weighted sampling we can produce a fixed number (i.e., using a reservoir) of exemplars with an equal expected number of spans per bucket.

This simple approach uses two passes, documented in the README of the https://github.com/lightstep/varopt repository. The more complicated approach can do this approximately in a single pass, which I also discussed briefly in today's Sampling SIG. See this draft. I will elaborate on this in a prototype exemplar reservoir tail sampler based on Varopt.

@jsuereth jsuereth marked this pull request as ready for review August 17, 2021 13:40
@jsuereth jsuereth requested review from a team August 17, 2021 13:40
@jsuereth
Copy link
Contributor Author

@jmacd I agree I think we can do way better than status quo, especially for high-fidelity histograms. I re-tweaked the wording to leave room for an exemplar reservoir sampling specification when those land.

@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Aug 25, 2021
@jsuereth jsuereth added area:sdk Related to the SDK spec:metrics Related to the specification/metrics directory and removed Stale labels Aug 25, 2021
specification/metrics/sdk.md Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
@jsuereth jsuereth changed the title First cut at exemplar spec. Metric Exemplars SDK Specification Aug 25, 2021
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I've left some minor comments.

specification/metrics/sdk.md Outdated Show resolved Hide resolved
The "offer" method SHOULD accept measurements, including:

- value
- `Attributes`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above

specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
@@ -128,6 +128,8 @@ are the inputs:
applies to [synchronous Instruments](./api.md#synchronous-instrument).
* The `aggregation` (optional) to be used. If not provided, a default
aggregation will be applied by the SDK. The default aggregation is a TODO.
* The `exemplar_reservoir` to use for storing exemplars.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be marked optional right?

jsuereth and others added 9 commits August 26, 2021 13:39
Co-authored-by: Reiley Yang <reyang@microsoft.com>
Co-authored-by: Reiley Yang <reyang@microsoft.com>
Co-authored-by: Reiley Yang <reyang@microsoft.com>
Co-authored-by: Reiley Yang <reyang@microsoft.com>
Co-authored-by: Reiley Yang <reyang@microsoft.com>
Co-authored-by: Aaron Abbott <aaronabbott@google.com>
specification/metrics/sdk.md Outdated Show resolved Hide resolved
@jmacd jmacd merged commit 307f20b into open-telemetry:main Aug 30, 2021
@jsuereth jsuereth deleted the wip-exemplar branch August 31, 2021 13:30
@reyang reyang mentioned this pull request Sep 10, 2021
@jsuereth jsuereth restored the wip-exemplar branch December 6, 2021 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:sdk Related to the SDK spec:metrics Related to the specification/metrics directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Define Exemplar requirements in the Metrics SDK spec
6 participants