client_rust
client_rust copied to clipboard
*: Move to `parking_lot` and thus make `owning_ref` obsolete
Before proemtheus-client
would use the owning_ref
crate to map the target
of a std::sync::RwLockReadGuard
. owning_ref
has multiple unsoundness
issues, see https://rustsec.org/advisories/RUSTSEC-2022-0040.html. Instead of
replacing owning_ref
with a similar crate, we switch to locking via
parking_lot
which supports the above mapping natively.
Fixes https://github.com/prometheus/client_rust/issues/77
I wonder whether moving to parking_lot
has any implications on this crate's support for WASM.
Gathering the WASM experts here. Do you have any opinions on this? Do we need to pipe through any feature flags?
@PhilippGackstatter, @thomaseizinger, @mriise, @stebalien
For the record, this patch does not seem to have a significant impact on benchmarks:
➜ critcmp master parking_lot
group master parking_lot
----- ------ -----------
counter family with Vec<(String, String)> label set 1.00 150.9±1.46ns ? ?/sec 1.02 153.9±3.00ns ? ?/sec
counter family with custom type label set 1.00 40.4±0.30ns ? ?/sec 1.11 44.9±4.67ns ? ?/sec
encode 1.07 26.5±0.16ms ? ?/sec 1.00 24.8±1.10ms ? ?/sec
➜ grep -m 1 'model name' /proc/cpuinfo
model name : Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
That said, I don't think the benchmarks are at a point, where I would say that they represent real world use-cases.
I wonder whether moving to
parking_lot
has any implications on this crate's support for WASM.Gathering the WASM experts here. Do you have any opinions on this? Do we need to pipe through any feature flags?
@PhilippGackstatter, @thomaseizinger, @mriise, @Stebalien
Not sure if I'd consider myself a WASM expert :see_no_evil:
Do we currently support compiling to WASM? Looking through the CI config, it doesn't seem to be or at least it is not verified? Adding wasm32-unknown-unknown
should at least give us some confidence here although we should bear in mind that often, things compile to wasm but don't actually work at runtime. To be really sure, we would have to add wasm_bindgen_test
based tests.
If WASM support is something we want to guarantee, I think we should make a separate PR that adds the target in the CI cross-compile config, merge that first and see if this PR is affected by it.
@mxinden and @thomaseizinger , I am a new contributor to this project, I am interested in knowing why we should or shouldn't support WASM ? Why WASM for a client that exports metrics? I am not able to understand the usage of WASM here, can you explain!
@mxinden and @thomaseizinger , I am a new contributor to this project, I am interested in knowing why we should or shouldn't support WASM ? Why WASM for a client that exports metrics? I am not able to understand the usage of WASM here, can you explain!
WASM is becoming a more and more relevant deployment target and no longer necessarily means "browser" or "web".
See for example:
- https://github.com/fermyon/spin
- https://github.com/lunatic-solutions/lunatic
It is completely feasible that users would want to collect metrics in applications written for either of these two platforms in which case they have to compile the Rust client for prometheus to WASM.
Note: It looks like parking_lot actually has WASM support. When compiled without (experimental) WASM-atomic support, it just panics when trying to park, but that's fine.
If WASM support is something we want to guarantee, I think we should make a separate PR that adds the target in the CI cross-compile config, merge that first and see if this PR is affected by it.
I've made a PR for that here: https://github.com/prometheus/client_rust/pull/79
Note: It looks like parking_lot actually has WASM support. When compiled without (experimental) WASM-atomic support, it just panics when trying to park, but that's fine.
Adding to this:
The wasm32-unknown-unknown target is only fully supported on nightly with -C target-feature=+atomics in RUSTFLAGS and -Z build-std passed to cargo. parking_lot will work mostly fine on stable, the only difference is it will panic instead of block forever if you hit a deadlock. Just make sure not to enable -C target-feature=+atomics on stable as that will allow wasm to run with multiple threads which will completely break parking_lot's concurrency guarantees.
https://github.com/Amanieu/parking_lot#nightly-vs-stable
Unless there are any objections, I will merge and release this tomorrow or next week. Thanks for the input everyone!