Added Counter<u32, AtomicU32> implementation

[navaati]

This is useful for platforms that don’t support 64bit, such as a number of std-enabled networked MCUs.

I have unfortunately not been able to test the protobuf part, as I’m unable to build it (something about protoc failed: google/protobuf/timestamp.proto: File not found.). Hope CI will test it !

[mxinden]

Looks good to me. Thank you for the work!

Mind adding bumping the patch version in the root Cargo.toml and writing a CHANGELOG.md entry? I can then cut a new release right after merge.

[navaati]

I have more to come regarding 32-bit wide datatypes (this one was the uncontroversial low hanging fruit), so let’s not cut a release right now to cut on overhead. I’ll add a changelog though.

[navaati]

Interestingly there is a test compile fail on this one, that I had not seen before:

error[E0277]: the trait bound `i32: EncodeCounterValue` is not satisfied
   --> src/encoding/text.rs:890:29
    |
890 |                     counter.metric_type(),
    |                             ^^^^^^^^^^^ the trait `EncodeCounterValue` is not implemented for `i32`, which is required by `ConstCounte<{integer}>: EncodeMetric`
    |
    = help: the following other types implement trait `EncodeCounterValue`:
              u32
              u64
              f64
note: required for `ConstCounter<i32>` to implement `EncodeMetric`
   --> src/metrics/counter.rs:209:9
    |
209 | impl<N> EncodeMetric for ConstCounter<N>
    |         ^^^^^^^^^^^^     ^^^^^^^^^^^^^^^
210 | where
211 |     N: crate::encoding::EncodeCounterValue,
    |        ----------------------------------- unsatisfied trait bound introduced here

For more information about this error, try `rustc --explain E0277`.
error: could not compile `prometheus-client` (lib test) due to 1 previous error

I suspect that it’s because integer literal will select the only available type when type inference is unambiguous (when there was only u64) but will default to i32 otherwise (see Rust Reference), and introducing u32 beside u64 makes it ambiguous.

Will fix that.

[navaati]

Soooo… I’ve fixed it in the simplest way, but it raises several questions, as this innocent looking commit could actually be a breaking change x). Putting the PR as draft for now and let me share my analysis.

First, let’s scope it: only ConstCounter is affected, which is a very particular usecase not really on the "core user journey". This is because only in ConstCounter do you give a literal argument to ::new(), for others like Gauge or Counter you don’t, typically you use Default::default() which doesn’t take an argument so the default generic argument in the type declaration (for example u64 in pub struct Counter<N = u64, A = AtomicU64> {) is taken. However for ConstCounter, the i32 of the literal "wins" over the generic type argument.

So if we merge as is, it’s a hard breaking change (compilation fail) for anyone using ConstCounter::new() who didn’t manually specify the u64 integer literal qualifier suffix (like I did in the current test fixes). Nobody does that, so everyone using ConstCounter will get a compilation break, that would clearly need a major version bump, ugh.

Another solution is adding to this PR, before the current commit, a commit to support i32. It would be a "soft breaking change", as in everyone using ConstCounter with an unspecified literal will see their memory layout silently go from u64 to i32, but it will continue compiling and working. And there is no risk of them experiencing an overflow that they didn’t before upon increasing the value because ConstCounter is, well, constant, so there is never any increase operation on this number, once it’s passed to ::new() it won’t change. The only possible breakage would be if someone calls ConstCounter::new(3_000_000_000) (or any literal value greater than 2^31), in which case they would get a compile break on cargo update and would need to add the integer literal qualifier suffix (a trivial and obvious fix). I let you judge whether it’s common enough to warrant a major version bump or not.

@mxinden what do you think ?

[mxinden]

Thank you for the investigation.

I would prefer playing it safe and releasing it as a breaking change, i.e. a minor version bump (v0.23.0). Given the widespread use of @dependabot, I don't think this creates too much work for downstream users.

I suggest calling out in the changelog entry, that this is a minor breaking change only.

Again, thank you for the work!

[navaati]

Hi.

Aaaah, I hadn’t realized you were still in the 0.x.y phase, so it’s less disruptive that what I imagined indeed. And that’s a good thing, because I realized that my idea of adding a i32 instance is, uh, not so great: i32 for a Counter doesn’t make much sense, as Counter should never be negative !

Ok so if we can go ahead and break a bit, I’ll prepare a series of commits in order to add all missing 32-bit support once and for all and break only once, after which we can cut a release.

Thanks for your answer, now it’s clearer !

[mxinden]

@navaati merge window for 0.23 is open now. I.e. ready to merge breaking changes.

[navaati]

Hi, I see you merged #173, which I guess supersedes my PR (ugh, I would I saved time if I’d seen it…) ? At least it already introduces the breaking change of needing the u64 suffix on ConstCounter::new. Let me rebase and see if anything is missing.

[navaati]

Ok, got it ! Where I implemented EncodeCounterValue for <32 bit type> by creating a method CounterValueEncoder::encode_<32 bit type> , he simply implemented it by calling CounterValueEncoder::encode_<64 bit type>(*self as <64 bit type>), which is… much less code and much smarter, the encode path is not performance critical anyway so going through the 64-bit emulation is not a problem.

Cool ! Let’s close this PR, superseded by #173.

There might be some work left for actual f32 support (the Atomic part) but that’s for another PR.