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

Revise the exemplar default reservoirs #3627

Merged
merged 4 commits into from
Aug 1, 2023

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Jul 27, 2023

  • Make it normative that an SDK is recommended to include both types of default reservoirs.
  • Make the description of both default reservoirs their own sub-sections.
  • Correct the "SimpleExemplarReservoir" section to be titled "SimpleFixedSizeExemplarReservoir".
  • Loosen the SimpleFixedSizeExemplarReservoir to allow any uniformly-weighted sampling algorithm instead of just the naive one that is not optimal.
    • The current algorithm has an asymptotic running time of O(n). There are other sampling algorithms that produce equivalent results and have more optimal asymptotic running times. Therefore, do not restrict implementations to only implementing the inefficient algorithm.

@MrAlias MrAlias added the spec:metrics Related to the specification/metrics directory label Jul 27, 2023
@MrAlias MrAlias requested review from a team July 27, 2023 16:17
Make it normative that an SDK is recommended to include both types of
default reservoirs.

Make the description of both default reservoirs their own sub-sections.

Correct the "SimpleExemplarReservoir" section to be titled
"SimpleFixedSizeExemplarReservoir".

Loosen the SimpleFixedSizeExemplarReservoir to allow any
uniformly-weighted sampling algorithm instead of just the naive one that
is not optimal.
Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

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

Overall like these improvements, just two minor nits.

specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
@jsuereth jsuereth merged commit baecbb8 into open-telemetry:main Aug 1, 2023
6 checks passed
@MrAlias MrAlias deleted the exemplar-res-default branch August 1, 2023 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:metrics Related to the specification/metrics directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants