-
Notifications
You must be signed in to change notification settings - Fork 158
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
Add support for tracing::Span::record
ed fields in metrics-tracing-context
#408
Conversation
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.
I still need to read over the documentation changes, but left at least one bit of feedback on a required change.
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.
Overall, this is looking good and just needs a little more polish. I still haven't looked fully at the unit tests or benchmarks, since I just wanted to make sure any relevant code changes are in place before thinking about that stuff.
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.
One more small functional change we need to make, then I'll do the final review over the tests.
Appreciate your patience so far on this as my time has been a bit limited lately. 😓
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.
Looking good, but it still seems like we don't quite have the label priority logic correct.
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.
Alright, this is very close, but it looks like we changed one bit of filtering logic during some of the refactoring that just needs to be changed back.
Once that's fixed (and the other bit I made a note about in Cargo.toml
) this should be ready to merge. 🎉
Pretty sad perf stats. Ready for a review though. Running benches/layer.rs (/home/zohnannor/dev/metrics/target/release/deps/layer-5bf703a34fc6c1b2)
Gnuplot not found, using plotters backend
layer/base case time: [923.19 ps 923.33 ps 923.48 ps]
change: [-16.774% -16.758% -16.741%] (p = 0.00 < 0.05)
Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
4 (4.00%) high mild
4 (4.00%) high severe
layer/no integration time: [928.33 ps 928.41 ps 928.52 ps]
change: [-14.183% -14.125% -14.074%] (p = 0.00 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
1 (1.00%) high mild
2 (2.00%) high severe
layer/tracing layer only
time: [923.75 ps 923.78 ps 923.81 ps]
change: [-14.670% -14.622% -14.579%] (p = 0.00 < 0.05)
Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
6 (6.00%) high mild
2 (2.00%) high severe
layer/metrics layer only
time: [25.805 ns 26.988 ns 28.176 ns]
change: [+17.291% +21.416% +25.369%] (p = 0.00 < 0.05)
Performance has regressed.
layer/full integration time: [270.72 ns 270.85 ns 270.98 ns]
change: [+50.150% +50.248% +50.344%] (p = 0.00 < 0.05)
Performance has regressed.
Running benches/visit.rs (/home/zohnannor/dev/metrics/target/release/deps/visit-1cf93c53aa04b250)
Gnuplot not found, using plotters backend
visit/record_str time: [23.671 ns 23.686 ns 23.699 ns]
change: [+137.50% +138.60% +139.73%] (p = 0.00 < 0.05)
Performance has regressed.
visit/record_bool[true] time: [18.172 ns 18.179 ns 18.186 ns]
change: [+201.05% +201.53% +201.80%] (p = 0.00 < 0.05)
Performance has regressed.
Found 6 outliers among 100 measurements (6.00%)
1 (1.00%) low severe
1 (1.00%) low mild
4 (4.00%) high mild
visit/record_bool[false]
time: [18.097 ns 18.099 ns 18.101 ns]
change: [+199.13% +199.29% +199.44%] (p = 0.00 < 0.05)
Performance has regressed.
Found 11 outliers among 100 measurements (11.00%)
3 (3.00%) high mild
8 (8.00%) high severe
visit/record_i64 time: [25.666 ns 25.673 ns 25.681 ns]
change: [+92.582% +92.723% +92.862%] (p = 0.00 < 0.05)
Performance has regressed.
Found 6 outliers among 100 measurements (6.00%)
1 (1.00%) low severe
2 (2.00%) low mild
1 (1.00%) high mild
2 (2.00%) high severe
visit/record_u64 time: [25.482 ns 25.488 ns 25.495 ns]
change: [+94.980% +95.075% +95.167%] (p = 0.00 < 0.05)
Performance has regressed.
Found 2 outliers among 100 measurements (2.00%)
1 (1.00%) high mild
1 (1.00%) high severe
visit/record_debug time: [163.06 ns 163.16 ns 163.26 ns]
change: [+21.900% +22.026% +22.130%] (p = 0.00 < 0.05)
Performance has regressed.
Found 5 outliers among 100 measurements (5.00%)
1 (1.00%) low severe
1 (1.00%) low mild
3 (3.00%) high mild
visit/record_debug[bool]
time: [39.089 ns 39.096 ns 39.104 ns]
change: [+73.869% +74.077% +74.278%] (p = 0.00 < 0.05)
Performance has regressed.
Found 6 outliers among 100 measurements (6.00%)
1 (1.00%) low severe
3 (3.00%) low mild
1 (1.00%) high mild
1 (1.00%) high severe
visit/record_debug[i64] time: [46.002 ns 46.010 ns 46.018 ns]
change: [+67.113% +67.556% +67.989%] (p = 0.00 < 0.05)
Performance has regressed.
Found 5 outliers among 100 measurements (5.00%)
2 (2.00%) high mild
3 (3.00%) high severe
visit/record_debug[u64] time: [43.717 ns 43.731 ns 43.746 ns]
change: [+62.503% +62.569% +62.631%] (p = 0.00 < 0.05)
Performance has regressed.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high mild |
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.
Woohoo, this is ready to merge. 🎉
The performance numbers are kind of "eh, whatever": a big increase, but it's still so fast overall that it shouldn't be a problem for users of this crate.
Cool! Thank you for review! When should we expect the release? 🙂 |
I would say within the next two weeks. There's a lot of changes queued up on |
@tobz friendly ping~ |
👋🏻 Sorry, life has been very busy 😭 ... although I'm finally on holiday vacation starting today. 😀 Going to try and see if I can get release notes written up today and then cut releases for everything. |
* fix prometheus metric name and label key sanitizer (metrics-rs#296) Co-authored-by: Toby Lawrence <[email protected]> * metrics-util: add ability to collect metrics on a per-thread basis via DebuggingRecorder (metrics-rs#299) Signed-off-by: Toby Lawrence <[email protected]> * (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 <[email protected]> * 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] <[email protected]> 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] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Toby Lawrence <[email protected]> * 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] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Toby Lawrence <[email protected]> * 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 <[email protected]> * 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] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Toby Lawrence <[email protected]> * 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] <[email protected]> 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 <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Shaoyuan CHEN <[email protected]> Co-authored-by: Toby Lawrence <[email protected]> Co-authored-by: Toby Lawrence <[email protected]> Co-authored-by: nils <[email protected]> Co-authored-by: Dan Wilbanks <[email protected]> Co-authored-by: Daniel Nelson <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Lucas Kent <[email protected]> Co-authored-by: Fredrik Enestad <[email protected]> Co-authored-by: Christopher Hunt <[email protected]> Co-authored-by: zohnannor <[email protected]> Co-authored-by: Sinotov Aleksandr <[email protected]> Co-authored-by: Jacob Kiesel <[email protected]> Co-authored-by: C J Silverio <[email protected]> Co-authored-by: CinchBlue <[email protected]> Co-authored-by: JasonLi <[email protected]> Co-authored-by: Mostafa Elhemali <[email protected]> Co-authored-by: Harry Barber <[email protected]> Co-authored-by: Qingwen Zhao <[email protected]> Co-authored-by: david-perez <[email protected]> Co-authored-by: Lucio Franco <[email protected]> Co-authored-by: Nicolas Stinus <[email protected]> Co-authored-by: Valeriy V. Vorotyntsev <[email protected]>
Released as Thanks again for your contribution. ❤️ |
I wondered for the reason of why
record
ed fields are not added to theLabels
and found none. Implemented it as justLabel::extend_from_labels
call in aLayer::on_record
method. For the tests I am not sure, maybe I missed some corner case, but I can't see such a case.P.S. Should I ignore falling CI checks?