Repository navigation
Change from dtoa to zmij - #369
Conversation
2fd9523 to
bd61497
Compare
krisztianfekete
left a comment
krisztianfekete
left a comment
There was a problem hiding this comment.
Thanks,
Before merging:
- #368 is about to land and adds a few dtoa calls in
prometheus_text.rs. Could you rebase after it merges and switch those to zmij too? - Please add a test with fixed expected strings (see my inline comment).
- Please add a CHANGELOG.md line under Changed saying float output now uses the shortest form, so some f64 label values (including le) can look different.
I'd like to ship this together with #368 in 0.26.
| impl EncodeLabelValue for f64 { | ||
| fn encode(&self, encoder: &mut LabelValueEncoder) -> Result<(), std::fmt::Error> { | ||
| encoder.write_str_unescaped(dtoa::Buffer::new().format(*self)) | ||
| encoder.write_str_unescaped(zmij::Buffer::new().format(*self)) |
There was a problem hiding this comment.
This is also used for histogram le labels, so some label strings will change.
For example, with linear_buckets(0.1, 0.1, 20):
- today:
le="0.30000000000000007" - with this PR:
le="0.30000000000000004"
Very small and very large values change too: 0.000001 becomes 1e-6, and 10000000000000000.0 becomes 1e+16.
The number is the same, but the label is different, so Prometheus sees a new series. I think the change is fine. We just need to tell users (see my top comment).
There was a problem hiding this comment.
I added a test for histogram le labels in 4df9df3
| + "# UNIT my_counter_with_exemplar_seconds seconds\n" | ||
| + "my_counter_with_exemplar_seconds_total 2 # {user_id=\"99\"} 1.0 " | ||
| + dtoa::Buffer::new().format(now.duration_since(UNIX_EPOCH).unwrap().as_secs_f64()) | ||
| + zmij::Buffer::new().format(now.duration_since(UNIX_EPOCH).unwrap().as_secs_f64()) |
There was a problem hiding this comment.
This test builds the expected value with zmij too, so it can't catch a format change.
Could you add one small test with fixed expected strings? For example: 1e-6, 1e16, 0.1 + 0.2, and one le from linear_buckets. Then a future zmij update can't change our output without us noticing.
|
#368 is merged now. Could you rebase on master? |
|
Ill have a look at it this week! |
6fcbf65 to
f84482e
Compare
|
@krisztianfekete I have implemented all requested changes. Feel free to modify if there are any minor changes needed. |
f84482e to
fee89e7
Compare
krisztianfekete
left a comment
krisztianfekete
left a comment
There was a problem hiding this comment.
Thanks, added two small comments, then we can merge this!
| } | ||
|
|
||
| #[test] | ||
| fn encode_histogram_fixed_le_labels() { |
There was a problem hiding this comment.
Thanks for the test! Note that in prometheus_text, le uses GoFloat, not zmij`. This test also passes with the old dtoa code, so it doesn't catch your change.
The le that changes is in the OpenMetrics encoder in text.rs. Could you move it there?
There was a problem hiding this comment.
0.26.0 was released in the meantime, can you put it under a new ## [Unreleased] section?
krisztianfekete
left a comment
krisztianfekete
left a comment
There was a problem hiding this comment.
Thanks, the tests look good now! One last thing: the changelog diff still removes the PR 370 entry (and the [PR 370] link) from the 0.26.0 section. Could you add them back? After that I'll merge.
Oeps! Fixed in a4b28bb |
|
Sign the DCO please. |
a4b28bb to
e8634ee
Compare
krisztianfekete
left a comment
krisztianfekete
left a comment
There was a problem hiding this comment.
Thanks, CI is green now! Your earlier commit a4b28bb had everything right, so please push it as one signed commit on top of master. That puts the changelog line under [Unreleased] and drops the import-style churn, and we'll do the parser fix in encode_flushes_large_output_in_chunks in a separate PR.
Signed-off-by: Bent Engbers <bentengbers@dreamsolution.nl>
c810bd1 to
bc7a041
Compare
Both crates are written by dtolnay:
https://github.com/dtolnay/dtoa
https://crates.io/crates/dtoa
https://github.com/dtolnay/zmij
https://crates.io/crates/zmij
Zmij seems more commonly used and should be faster According to dtolnay's own benchmarks (although running the benchmarks in this project on my machine did not show any significant changes). Zmij was already part of this project (See changes in the Cargo.lock).