Skip to content

feat(encoding): add impls for Encode*Value#173

Merged
mxinden merged 4 commits intoprometheus:masterfrom
brocaar:fix_issue_172
Jul 17, 2024
Merged

feat(encoding): add impls for Encode*Value#173
mxinden merged 4 commits intoprometheus:masterfrom
brocaar:fix_issue_172

Conversation

@brocaar
Copy link
Contributor

@brocaar brocaar commented Oct 28, 2023

With these changes, I no longer get the error:

trait EncodeCounterValue is not implemented for u32

when compiling to mips.

I have also added additional trait implementations for the other types. The reason to cast from 32 to 64 bit values is that in the end, it is casted to 64 bit values anyway, because these are the types that must be used: https://github.com/prometheus/client_rust/blob/master/src/encoding/proto/openmetrics_data_model.proto. This way, there is no need to implement encoder.encode_u32 ... methods.

I believe this is not catched by the tests, because the library itself compiles fine to mips, until you start implementing it. Unfortunately the cross-compile check does not run the tests. I did a quick test changing the cargo check ... to cargo check --target=${{ matrix.target }} --tests, but this will return errors because the dev-dependency pyo3 a bit more than just the Rust toolchain for mips is required.

Fixes #172.

@brocaar
Copy link
Contributor Author

brocaar commented Oct 28, 2023

As a reference, this is the error I get when using cargo check ... --tests:

error: failed to run custom build command for `pyo3-ffi v0.20.0`

Caused by:
  process didn't exit successfully: `/home/runner/work/client_rust/client_rust/target/debug/build/pyo3-ffi-4fe3cdc12a0f9dda/build-script-build` (exit status: 1)
  --- stdout
  cargo:rerun-if-env-changed=PYO3_CROSS
  cargo:rerun-if-env-changed=PYO3_CROSS_LIB_DIR
  cargo:rerun-if-env-changed=PYO3_CROSS_PYTHON_VERSION
  cargo:rerun-if-env-changed=PYO3_CROSS_PYTHON_IMPLEMENTATION
  cargo:rerun-if-env-changed=PYO3_NO_PYTHON

  --- stderr
  error: PYO3_CROSS_PYTHON_VERSION or an abi3-py3* feature must be specified when cross-compiling and PYO3_CROSS_LIB_DIR is not set.
warning: build failed, waiting for other jobs to finish...
error: failed to run custom build command for `zstd-sys v2.0.9+zstd.1.5.5`

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sorry for the delay here.

Thank you for debugging and sending a patch!

I would appreciate any additions to the CI to prevent this from happening in the future.

src/encoding.rs Outdated

impl EncodeCounterValue for i32 {
fn encode(&self, encoder: &mut CounterValueEncoder) -> Result<(), std::fmt::Error> {
encoder.encode_u64(*self as u64)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
encoder.encode_u64(*self as u64)
encoder.encode_i64(*self as u64)

Copy link
Contributor Author

@brocaar brocaar Nov 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure? https://github.com/prometheus/client_rust/blob/master/src/encoding.rs#L571

impl<'a> CounterValueEncoder<'a> {
    fn encode_f64(&mut self, v: f64) -> Result<(), std::fmt::Error> {
        for_both_mut!(self, CounterValueEncoderInner, e, e.encode_f64(v))
    }

    fn encode_u64(&mut self, v: u64) -> Result<(), std::fmt::Error> {
        for_both_mut!(self, CounterValueEncoderInner, e, e.encode_u64(v))
    }
}

src/encoding.rs Outdated
}
}

impl EncodeCounterValue for i32 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A counter can never be negative. Why implement EncodeCounterValue for i32?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, removing this as you suggests raises the following error:

 error[E0277]: the trait bound `i32: EncodeCounterValue` is not satisfied
Error:    --> src/encoding/text.rs:875:29
    |
875 |                     counter.metric_type(),
    |                             ^^^^^^^^^^^ the trait `EncodeCounterValue` is not implemented for `i32`
    |
    = help: the following other types implement trait `EncodeCounterValue`:
              u32
              u64
              f32
              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

Fixes prometheus#172.

Signed-off-by: Orne Brocaar <info@brocaar.com>
@brocaar
Copy link
Contributor Author

brocaar commented Nov 21, 2023

I would appreciate any additions to the CI to prevent this from happening in the future.

I do not have the bandwidth to work on this short-term, but most likely you would like to use something like:
https://github.com/cross-rs/cross

@brocaar
Copy link
Contributor Author

brocaar commented Dec 5, 2023

@mxinden I'm looking forward to your feedback on this :-)

@brocaar
Copy link
Contributor Author

brocaar commented Dec 21, 2023

Hi @mxinden I'm still looking forward to your feedback :-)

mxinden added 3 commits July 17, 2024 09:00
Signed-off-by: Max Inden <mail@max-inden.de>
Signed-off-by: Max Inden <mail@max-inden.de>
@mxinden mxinden changed the title Fix missing trait implementations for value encode. feat(encoding): add impls for Encode*Value Jul 17, 2024
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the work and once again, sorry for the delay.

@mxinden mxinden merged commit 87442a2 into prometheus:master Jul 17, 2024
navaati added a commit to navaati/prometheus_client_rust that referenced this pull request Jul 17, 2024
navaati added a commit to navaati/prometheus_client_rust that referenced this pull request Jul 17, 2024
Signed-off-by: Léo Gillot-Lamure <leo.gillot@navaati.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mips: EncodeCounterValue implementation missing for u32?

2 participants