Having a default of std-atomics is problematic for 32 bit archs

[huntc]

Further to https://github.com/metrics-rs/metrics/pull/313, I believe we have a problem given that std-atomics are a default feature for the metrics crate. As with any feature, you cannot "unset" it. Therefore, when depending on, say, metrics-utils, the default feature will be enabled no matter what your target is.

The std-atomics default was added back in for https://github.com/metrics-rs/metrics/commit/65d965c10591373cf7348570c45c1d156fb5f92c.

Perhaps I'm missing something though? Or would it be better to have the std-atomics feature as the default for the other "leaf" crates?

That said, shouldn't the portable atomic library enable Atomic64 where it is available i.e. do we need the std-atomics feature at all?

For completeness, I'm trying to depend on both metrics-exporter-prometheus and metrics-util.

[tobz]

So, at a high level: I don't intend to force everyone using metrics to use portable-atomic. Practically speaking, does it provide equivalent behavior to std for architectures that don't need shimmed atomics? Yes. Do I want to force that dependency on them, though? No.

That's the main reason I added the std-atomics feature as a default feature.

All of that said, we should be able to just disable the default features for leaf crates that depend on metrics, which would only allow the portable-atomic-based impls come through.

[huntc]

So, at a high level: I don't intend to force everyone using metrics to use portable-atomic. Practically speaking, does it provide equivalent behavior to std for architectures that don't need shimmed atomics? Yes. Do I want to force that dependency on them, though? No.

Isn’t portable atomic already mandatory as a dependency?

Also, I think we should either trust portable atomic or not. I’m happy with not, but we will then need to reconsider the use of 64 bit operations and/or perhaps the applicability of metrics for 32 bit archs. Wdyt?

[tobz]

I do trust portable-atomic, but forcing every implementor to use it just... doesn't make sense. There's no technical reason why the atomics from the standard library aren't good enough, if that's what someone prefers.

I do realize that this perhaps feels like circular logic/silliness -- "you told me portable-atomic was good enough, and now it's not?" -- but I do believe this is the best middleground for now, so long as we make metrics leaf crates opt out of the default features: everyone gets the same available impls for CounterFn, etc, as they've had since the traits were introduced, but metrics leaf crates can scope themselves down so they always work seamlessly on 32- or 64-bit targets.... and external crates can make whatever decision they choose re: what types to use, what archs to support, etc.

[huntc]

I do trust portable-atomic, but forcing every implementor to use it just... doesn't make sense. There's no technical reason why the atomics from the standard library aren't good enough, if that's what someone prefers.

Sure, but aren’t we getting the std lib atomics still with portable atomic when supported by the target? I believe we are but my understanding could be off.

I do realize that this perhaps feels like circular logic/silliness -- "you told me portable-atomic was good enough, and now it's not?" --

No problem at all! Just seeking the best outcome. I’m also happy for the decision to be that 32 bit support is out and clearly stated as a non-goal.

but I do believe this is the best middleground for now, so long as we make metrics leaf crates opt out of the default features: everyone gets the same available impls for CounterFn, etc, as they've had since the traits were introduced, but metrics leaf crates can scope themselves down so they always work seamlessly on 32- or 64-bit targets.... and external crates can make whatever decision they choose re: what types to use, what archs to support, etc.

I’m unsure if you can “opt out” of the default features with cargo.

Unless you mean to remove the default from metrics, and make it the default from the leaf crates then, on the basis that features are additive?

[huntc]

Hey @tobz - I'm happy to close out this issue if you'd prefer. I'm actually thinking of experimenting with an open-metrics implementation that is suitable for no-std/no-alloc environments and thus more suitable than me. I therefore don't want to bend your original requirements here.

[tobz]

Sure, but aren’t we getting the std lib atomics still with portable atomic when supported by the target? I believe we are but my understanding could be off.

Under the hood, essentially, but not at a type level. Our implementation of CounterFn for portable_atomic::AtomicU64 doesn't translate to support std::sync::atomic::AtomicU64 so long as you're on a platform where std::sync::atomic::AtomicU64 exists in the standard library.

If we provide both, with the standard library ones behind a feature flag, then we always know that our maximal architecture support footprint is whatever portable_atomic supports.. but maybe more practically, it meant that there was less churn, API-wise, for users upgrading, since we didn't force everyone to have to now use portable_atomic instead.

I’m unsure if you can “opt out” of the default features with cargo.

You can, by setting default-features to false in Cargo.toml:

[dependencies]
metrics = { version = "0.20.1", default-features = false, features = ["maybe_one_of_the_defaults"] }

Overall, this really should just boil down to using default-features = false (plus adding back whatever features are actually needed) in the leaf crates. My thought/expectation is that portable_atomic is likely to make it back upstream, in some form, in a year or two. That's why I want to keep the stdlib atomics around (at least flagged, so folks with your constraints can opt out) because I feel like eventually we'll be able to drop portable_atomic... and we won't have needed to change things twice.

[huntc]

I’m unsure if you can “opt out” of the default features with cargo.

You can, by setting default-features to false in Cargo.toml:

[dependencies]
metrics = { version = "0.20.1", default-features = false, features = ["maybe_one_of_the_defaults"] }

I tried that and couldn't get it to work. Perhaps I was doing something wrong.

[tobz]

Hmmm, that's really weird.

I'll take a shot today at putting my own suggestion into practice and try and see what's going on there.

[tobz]

I believe this is resolved as of #347.