-
Notifications
You must be signed in to change notification settings - Fork 84
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
Conversation
As a reference, this is the error I get when using
|
3fd3a1b
to
a96a61a
Compare
There was a problem hiding this 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
encoder.encode_u64(*self as u64) | |
encoder.encode_i64(*self as u64) |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
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 <[email protected]>
a96a61a
to
53d02ba
Compare
I do not have the bandwidth to work on this short-term, but most likely you would like to use something like: |
@mxinden I'm looking forward to your feedback on this :-) |
Hi @mxinden I'm still looking forward to your feedback :-) |
…to fix_issue_172
Signed-off-by: Max Inden <[email protected]>
Signed-off-by: Max Inden <[email protected]>
Encode*Value
There was a problem hiding this 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.
Signed-off-by: Léo Gillot-Lamure <[email protected]>
With these changes, I no longer get the error:
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 ...
tocargo check --target=${{ matrix.target }} --tests
, but this will return errors because the dev-dependencypyo3
a bit more than just the Rust toolchain for mips is required.Fixes #172.