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.

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

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

Merged
merged 4 commits into from
Jul 17, 2024

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

@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 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
10 checks passed
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
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