Quantile Remapping Fix #304
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 3 commits
May 27, 2022 19:21
DDSketch 0.1.3 resolves an issue with the rank calculation that could cause the sketch to return the value of neighboring ranks, instead of the one requested.
The 0.0 and 1.0 quantiles correspond to the minimum and maximum values of the distribution. This change returns these values when the 0.0 and 1.0 quantiles are requested. One benefit of doing so is that the minimum and maximum are the exact values for these quantiles, rather than estimates. Another benefit is that the 0.0 quantile can be considered a special case, since for any quantile, we return the maximum value below that quantile. Since there is no value below the 0.0 quantile, we instead return the minimum.
- Changes the rank calculation in Summary::quantile() to 'q * total'
from 'floor(q * (total - 1))'. The 'floor' in the previous calculation
was implicit due to the cast to u64.
- Changes the condition for mapping to the 'negative', 'positive', or
'zeroes' sketch to be '<=' instead of '<'.
- Defers rounding the quantile to the nearest rank to the underlying
sketch. This change is not necessary for correctness, but could be
desireable. For example, deferring this operation could allow the
underlying sketch to perform interpolation.
- Fixes incorrect assertions in test_basics(), which pass with the
changes to the rank calculation and quantile mapping.
The previous method could incorrectly map a request for a value at a
particular rank to the adjacent rank in some cases. Consider a
distribution with the values {0.0, 1.0}. In this case, any request for a
quantile < 1.0 would map to the 'zeroes' sketch. for example, the rank
for q = 0.9 is computed as 'floor(0.9 * (2 - 1)) = floor(0.9) = 0'.
This would map to the 'zeroes' sketch while it should instead map to a
request for q = 0.8 in the positive sketch. The new method would give us
a rank of '0.9 * 2 = 1.8' which correctly maps to 0.8 in the positive
sketch.
The condition for mapping to the appropriate sketch now uses the '<='
operator instead of the '<' operator. Consider a request for q = 0.5,
with the distribution {-1.0, 1.0}. The '<=' operator ensures that this
request is mapped to q = 0.0 in the negative sketch (would be 1.0, but
we flip the distribution) rather than q = 0.0 in the positive sketch.
The mapping for q = 0.0 was handled as a special case in a prior commit
to allow for the updated mapping, as q = 0.0 would otherwise always map
to the negative sketch, whether or not the negative sketch had any
values.
Both of the cases described above are now covered (with a different
distribution) in test_basics().
tobz
approved these changes
May 30, 2022
tobz
added
C-util
|
Released in |
tobz
removed
the
S-awaiting-release
jvimal-eg
added a commit
to edgeguard-dev/metrics
that referenced
this pull request
Dec 24, 2023
* fix prometheus metric name and label key sanitizer (metrics-rs#296) Co-authored-by: Toby Lawrence <tobz@users.noreply.github.com> * metrics-util: add ability to collect metrics on a per-thread basis via DebuggingRecorder (metrics-rs#299) Signed-off-by: Toby Lawrence <toby@nuclearfurnace.com> * (cargo-release) version 0.12.1 * Improve handling of the global recorder instance (metrics-rs#302) This gets rid of the dangerous `static mut`, adds more comments about the code, relaxes the orderings and documents the unsoundness of the `clear` function, in addition to marking it unsafe. It implements a small once_cell-like abstraction. Co-authored-by: Toby Lawrence <toby@nuclearfurnace.com> * Fix `metrics::Cow` provenance issue (metrics-rs#303) * Quantile Remapping Fix (metrics-rs#304) * update changelogs prior to release * (cargo-release) version 0.19.0 * (cargo-release) version 0.13.0 * (cargo-release) version 0.11.0 * (cargo-release) version 0.10.0 * Update CHANGELOG.md * Update README.md * Update ci.yml * Update ci.yml * Add RollingSummary to prevent summary saturation (metrics-rs#306) * Update quanta requirement from 0.9.3 to 0.10.0 (metrics-rs#301) Updates the requirements on [quanta](https://github.com/metrics-rs/quanta) to permit the latest version. - [Release notes](https://github.com/metrics-rs/quanta/releases) - [Changelog](https://github.com/metrics-rs/quanta/blob/main/CHANGELOG.md) - [Commits](metrics-rs/quanta@v0.9.3...v0.10.0) --- updated-dependencies: - dependency-name: quanta dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Document CI enforced msrv in readme and rust-version fields (metrics-rs#311) * Change description to be SharedString (metrics-rs#312) * Update hashbrown requirement from 0.11 to 0.12 (metrics-rs#266) Updates the requirements on [hashbrown](https://github.com/rust-lang/hashbrown) to permit the latest version. - [Release notes](https://github.com/rust-lang/hashbrown/releases) - [Changelog](https://github.com/rust-lang/hashbrown/blob/master/CHANGELOG.md) - [Commits](rust-lang/hashbrown@v0.11.0...v0.12.0) --- updated-dependencies: - dependency-name: hashbrown dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Toby Lawrence <tobz@users.noreply.github.com> * Shims remaining AtomicU64 usage (metrics-rs#313) * Update parking_lot requirement from 0.11 to 0.12 (metrics-rs#268) Updates the requirements on [parking_lot](https://github.com/Amanieu/parking_lot) to permit the latest version. - [Release notes](https://github.com/Amanieu/parking_lot/releases) - [Changelog](https://github.com/Amanieu/parking_lot/blob/master/CHANGELOG.md) - [Commits](Amanieu/parking_lot@0.11.0...0.12.0) --- updated-dependencies: - dependency-name: parking_lot dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Toby Lawrence <tobz@users.noreply.github.com> * update changelogs * (cargo-release) version 0.13.1 * add std atomics handle support back + changelogs * (cargo-release) version 0.20.0 * (cargo-release) version 0.14.0 * (cargo-release) version 0.11.0 * changelog * (cargo-release) version 0.12.0 * (cargo-release) version 0.7.0 * update changelog * (cargo-release) version 0.6.0 * update changelog * (cargo-release) version 0.20.1 * Remove incorrect return info (metrics-rs#316) * Add a `KeyName` argument to `LabelFilter::should_include_label` (metrics-rs#342) * rewind * (cargo-release) version 0.13.0 * Use std sync primitives instead of parking_lot. (metrics-rs#344) * Use std::sync::Mutex instead of parking_lot::Mutex. * Bump MSRV to 1.60.0 for CI. Co-authored-by: Toby Lawrence <tobz@users.noreply.github.com> * clean up github CI workflow + rust-toolchain.toml to match quanta * update portable-atomic to 1.0 * bump to prost 0.11 + fix spelling issue in metrics-tracing-context * bump MSRV to 1.61 + update hashbrown/ahash deps * update termion/ordered-float and pin predicates-* to avoid stupid 1.64 MSRV * update syn * update quanta + clean up semver notation in cargo.toml * cleanup README wording for MSRV policy * Add white background to splash image (metrics-rs#348) * Install protoc in CI. * fix spelling error in CI workflow * Update ci.yml * no need to run CI against macOS/Windows specifically * Bring the metrics-observer protobufs up to date (metrics-rs#345) * Use global paths for macros (metrics-rs#358) * fix changes to fully qualified metrics crate ref change in macros * update changelog * update tui to 0.19 * bump numpy dep * impl std::error::Error for metrics_exporter_tcp::Error * changelog * tweak test to avoid unused code * rework 32 vs 64-bit arch atomics support + a lot of import consolidation/cleanup * const-ify some stuff + rewrite some comments for the global recorder init code * clean up clippy lints * bump MSRV to 1.61.0 * (cargo-release) version 0.7.0 * (cargo-release) version 0.21.0 * (cargo-release) version 0.15.0 * (cargo-release) version 0.14.0 * (cargo-release) version 0.8.0 * (cargo-release) version 0.12.0 * allow publishing * (cargo-release) version 0.2.0 * make it publishable pt 2 * push-gateway support authentication (metrics-rs#366) * update changelog + fix failing feature check test * (cargo-release) version 0.12.1 * feat(util): new helper type for recovering recorder after installing it (metrics-rs#362) * Update aho-corasick to 1.0. * Impl From<std::borrow::Cow> for KeyName (metrics-rs#378) * changelog * Add `Borrow` impl to `KeyName` (metrics-rs#381) * bump deps + clippy stuff * changelog * pin hashbrown to avoid MSRV bump * (cargo-release) version 0.15.1 * (cargo-release) version 0.21.1 * Add metadata to metrics (metrics-rs#380) * add https support in Prometheus gateway (metrics-rs#392) * migrate from procedural to declarative macros (metrics-rs#386) * Make `Unit` methods return `&'static str` where possible (metrics-rs#393) * simplify macros (metrics-rs#394) * Add support for `Arc<T>` to `metrics::Cow<'a, T>`. (metrics-rs#402) * Add support for `tracing::Span::record`ed fields in `metrics-tracing-context` (metrics-rs#408) * fix(prom): `RollingSummary` overflow panic (metrics-rs#423) * update CHANGELOG * (cargo-release) version 0.12.2 * Remove unneeded unsafe from test (metrics-rs#427) * Fix feature check in CI (metrics-rs#428) * update changelogs/release notes * fix unsafe/incorrect crossbeam-epoch usage in Block<T> * Add a clippy CI check (metrics-rs#416) (metrics-rs#417) * Try other resolved addresses if the first one fails (metrics-rs#429) * update changelog * Update quanta requirement from 0.11 to 0.12 (metrics-rs#396) Updates the requirements on [quanta](https://github.com/metrics-rs/quanta) to permit the latest version. - [Changelog](https://github.com/metrics-rs/quanta/blob/v0.12.0/CHANGELOG.md) - [Commits](metrics-rs/quanta@v0.11.0...v0.12.0) --- updated-dependencies: - dependency-name: quanta dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Toby Lawrence <tobz@users.noreply.github.com> * Update ordered-float requirement from 3.4 to 4.2 (metrics-rs#421) Updates the requirements on [ordered-float](https://github.com/reem/rust-ordered-float) to permit the latest version. - [Release notes](https://github.com/reem/rust-ordered-float/releases) - [Commits](reem/rust-ordered-float@v3.4.0...v4.2.0) --- updated-dependencies: - dependency-name: ordered-float dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * fix quantile api * missing clear * reintroduce old way to avoid big refactor --------- Signed-off-by: Toby Lawrence <toby@nuclearfurnace.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Shaoyuan CHEN <chensy20@mails.tsinghua.edu.cn> Co-authored-by: Toby Lawrence <tobz@users.noreply.github.com> Co-authored-by: Toby Lawrence <toby@nuclearfurnace.com> Co-authored-by: nils <48135649+Nilstrieb@users.noreply.github.com> Co-authored-by: Dan Wilbanks <78116928+dfwilbanks395@users.noreply.github.com> Co-authored-by: Daniel Nelson <daniel@wavesofdawn.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Lucas Kent <rubickent@gmail.com> Co-authored-by: Fredrik Enestad <fredrik@enestad.com> Co-authored-by: Christopher Hunt <huntchr@gmail.com> Co-authored-by: zohnannor <35764628+zohnannor@users.noreply.github.com> Co-authored-by: Sinotov Aleksandr <bratsinot@gmail.com> Co-authored-by: Jacob Kiesel <kieseljake@live.com> Co-authored-by: C J Silverio <ceejceej@gmail.com> Co-authored-by: CinchBlue <aytasato@gmail.com> Co-authored-by: JasonLi <lijingxuan92@126.com> Co-authored-by: Mostafa Elhemali <mostafa.elhemaly@gmail.com> Co-authored-by: Harry Barber <106155934+hlbarber@users.noreply.github.com> Co-authored-by: Qingwen Zhao <864593343@qq.com> Co-authored-by: david-perez <d@vidp.dev> Co-authored-by: Lucio Franco <luciofranco14@gmail.com> Co-authored-by: Nicolas Stinus <nicolas.stinus@gmail.com> Co-authored-by: Valeriy V. Vorotyntsev <valery.vv@gmail.com>
mnpw
pushed a commit
to mnpw/metrics
that referenced
this pull request
Mar 8, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR includes the following: