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.

Sign up for GitHub

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

Jump to bottom

Shims remaining AtomicU64 usage #313

Merged
merged 1 commit into from
Jul 20, 2022
Merged

Shims remaining AtomicU64 usage #313

merged 1 commit into from
Jul 20, 2022

Conversation

huntc
Copy link
Contributor

@huntc huntc commented Jul 11, 2022 •
edited
Loading

Shims the use of AtomicU64 in the main metrics lib so that we can use the library on MIPS and other 32 bit platforms.

Changes the atomic lib to portable-atomic as the atomic-shim no longer provides a complete implementation of the methods we require.

Tested building with target.mipsel-unknown-linux-musl.

Fixes #235

@huntc huntc marked this pull request as draft July 11, 2022 08:21
@huntc
Copy link
Contributor Author

huntc commented Jul 11, 2022

Needs some more work... some of the atomic methods being used aren't available on MIPS either...

@huntc
Copy link
Contributor Author

huntc commented Jul 11, 2022

The problem I'm faced with here is that fetch_max and fetch_update do not exist in the shimmed AtomicU64. I've created an issue there and will attempt a PR later in the week. Appreciate any other thinking on this too though. Thanks.

@huntc
Copy link
Contributor Author

huntc commented Jul 11, 2022 •
edited
Loading

One other thought: could we get away with 32 bit hash values for keys?

@tobz
Copy link
Member

tobz commented Jul 11, 2022

One other thought: could we get away with 32 bit hash values for keys?

We cannot, no: we need to be able to hold a u64 in order to satisfy std::hash::Hasher::finish.

@huntc
Copy link
Contributor Author

huntc commented Jul 11, 2022

One other thought: could we get away with 32 bit hash values for keys?

We cannot, no: we need to be able to hold a u64 in order to satisfy std::hash::Hasher::finish.

Could a 32 bit hash be used as an opt-in feature, or even more generally? We then cast the u64 to u32 for the atomic.

@tobz
Copy link
Member

tobz commented Jul 11, 2022

Could a 32 bit hash be used as an opt-in feature, or even more generally? We then cast the u64 to u32 for the atomic.

Doing so would imply that we need to drop entropy from the original 64-bit hash, which I don't want to do. Performance is a top priority of metrics, and adding a simple way to effectively/silently truncate the hash values for keys, possibly leading to an increased rate of hash collisions, doesn't seem worth it to me as a shortcut to make this change.

Another crate to try out might be portable-atomic: it's a bit more generalized, has fewer transitive dependencies than atomic-shim, and most importantly, should have support for AtomicU64::fetch_max and AtomicU64::fetch_update.

@huntc
Copy link
Contributor Author

huntc commented Jul 11, 2022

Another crate to try out might be portable-atomic: it's a bit more generalized, has fewer transitive dependencies than atomic-shim, and most importantly, should have support for AtomicU64::fetch_max and AtomicU64::fetch_update.

Thanks. I’ll take a look in the next few days.

@huntc
Shims remaining AtomicU64 usage
21e1e55 
Shims the use of `AtomicU64` in the main metrics lib so that we can use the library on MIPS and other 32 bit platforms.
@huntc huntc marked this pull request as ready for review July 16, 2022 02:29
@huntc
Copy link
Contributor Author

huntc commented Jul 16, 2022

Another crate to try out might be portable-atomic: it's a bit more generalized, has fewer transitive dependencies than atomic-shim, and most importantly, should have support for AtomicU64::fetch_max and AtomicU64::fetch_update.

I've now updated my PR to use portable-atomic. All appears well given a target.mipsel-unknown-linux-musl build.

@huntc
Copy link
Contributor Author

huntc commented Jul 19, 2022 •
edited
Loading

Hey @tobz - are you happy to merge? Thanks!

@tobz
Copy link
Member

tobz commented Jul 20, 2022 •
edited
Loading

Sorry, been a bit preoccupied lately.

Looking it over: I'm happier with this, yes. I'll merge it once CI passes.

@huntc
Copy link
Contributor Author

huntc commented Jul 20, 2022

Sorry, been a bit preoccupied lately.

Looking it over: I'm happier with this, yes. I'll merge it once CI passes.

Thanks! I'm unsure why that target build is failing though... any ideas?

@huntc
Copy link
Contributor Author

huntc commented Jul 20, 2022

FYI I tried building the x86_64-unknown-linux-gnu target locally on metrics-tracing-context and all was well...

@tobz
Copy link
Member

tobz commented Jul 20, 2022

Thanks! I'm unsure why that target build is failing though... any ideas?

Yeah, to be honest, I don't know why it's up in arms... but I have to go through a lot of my open-source crates and deal with issues related to a bunch of downstream dependencies switching to the 2021 Edition and breaking CI anyways (😓), so... I'll just merge this for now and do a more thorough pass locally.

@tobz tobz merged commit 6f043e8 into metrics-rs:main Jul 20, 2022
@tobz
Copy link
Member

tobz commented Jul 20, 2022

Thanks for your contribution!

@huntc
Copy link
Contributor Author

huntc commented Jul 20, 2022

Thanks for your contribution!

My pleasure. Thank you for your massive contribution with this library also!

@tobz tobz added C-util Component: utility classes and helpers. C-core Component: core functionality such as traits, etc. E-simple Effort: simple. T-enhancement Type: enhancement. S-awaiting-release Status: awaiting a release to be considered fixed/implemented. T-ergonomics Type: ergonomics. labels Jul 20, 2022
@tobz
Copy link
Member

tobz commented Jul 21, 2022

Released as metrics@v0.20.0, metrics-util@v0.14.0, metrics-exporter-prometheus@v0.11.0, and others.

@tobz tobz removed the S-awaiting-release Status: awaiting a release to be considered fixed/implemented. label Jul 21, 2022
@huntc huntc deleted the shim branch July 21, 2022 07:03
@taiki-e taiki-e mentioned this pull request Aug 8, 2022
Open
@huntc huntc mentioned this pull request Aug 17, 2022
Closed
jvimal-eg added a commit to edgeguard-dev/metrics that referenced this pull request Dec 24, 2023
@jvimal-eg @wx-csy
@tobz @Noratrieb @dfwilbanks395 @danielnelson @dependabot @rukai @fredr @huntc @zohnannor @BratSinot @Xaeroxe @ceejbot @CinchBlue @JasonLi-cn @mooso @hlbarber @david-perez @LucioFranco @nstinus @vvv
Merge changes from fork (#2)
9e8351e 
* 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
@huntc
Shims remaining AtomicU64 usage (metrics-rs#313)
a7865e7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tobz tobz tobz approved these changes

Assignees
No one assigned
Labels
C-core Component: core functionality such as traits, etc. C-util Component: utility classes and helpers. E-simple Effort: simple. T-enhancement Type: enhancement. T-ergonomics Type: ergonomics.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for 32-bit architectures?
2 participants
@huntc @tobz