feat(node): add fstrim datadir observability #3322
Conversation
There was a problem hiding this comment.
Thanks for also adding fstrim metrics for the data dir, @andrewbattat ! I made a (post-merge) comment on the previous PR that is somewhat unrelated to the changes you're making here, but it may have slipped through the cracks, so I thought I'd bring it up here also: #2953 (comment)
If you think it warrants some changes, those may fit better in a separate PR anyway.
| @@ -7,7 +7,7 @@ use std::time::Duration; | |||
| use tempfile::tempdir; | |||
|
|
|||
| #[test] | |||
| fn should_compare_f64() { | |||
| fn test_f64_comparison() { | |||
There was a problem hiding this comment.
Do these test_ prefixes add anything? I see them a lot but have the feeling it's just something people falsely do because it only adds noise.
There was a problem hiding this comment.
I'm happy to drop them. I find the "test" prefix much more readable than the "should" prefix, but maybe that's just me.
I know what you mean about the extra noise, but I do like that it makes each function signature a verb phase so then the name is the thing it is doing. The work this function is doing isn't "f64_comparison" or "should_compare_f64". It's doing "test_f64_comparison"
There was a problem hiding this comment.
+1, I think Rust conventions prefer you don't repeat yourself. I swear this shows up in the docs somewhere, but I couldn't seem to find the right link.
This shows up in enum naming:
IpAddr::V4 over IpAddr::IpV4
With a few exceptions, the get_ prefix is not used for getters in Rust code.
Do not include words in the name of a Cargo feature that convey zero meaning, as in use-abc or with-abc. Name the feature abc directly.
and crates:
Crate names should not use -rs or -rust as a suffix or prefix. Every crate is Rust! It serves no purpose to remind users of this constantly.
There was a problem hiding this comment.
Sounds good! Do you suggest just dropping all the test_ prefixes or reverting to the should_ prefix? I'd rather just drop the prefixes.
There was a problem hiding this comment.
I think we should use should_ only when it helps with understanding what the test does.
When the test tests a specific expected behavior, we can describe that behavior with should_. In other cases, the test tests a scenario (and sometimes multiple behaviors are tested), in these cases we can describe the scenario and don't need should.
e.g:
- without:
f64_comparison,compare_f64 - with:
should_not_panic(not_panicwould be a bad name) - in the middle:
f64_comparison_should_not_panic
There was a problem hiding this comment.
Updated unit test naming:
2db1249
rs/ic_os/fstrim_tool/src/tests.rs
Outdated
| let parsed_metrics_str = parsed_metrics.to_p8s_metrics_string(); | ||
| let default_str = FsTrimMetrics::default().to_p8s_metrics_string(); | ||
|
|
||
| assert_eq!(parsed_metrics_str, default_str); |
There was a problem hiding this comment.
Why not compare the actual metrics instead of their string representation?
There was a problem hiding this comment.
The parsing tests look good. However, it seems like we no longer test the output of to_p8s_metrics_string. It would make sense to have tests that parse that output (we have those) and other tests that recreate the output (where we check that the strings are formatted correctly).
There was a problem hiding this comment.
Added! a5c9a21
WDYT?
NODE-1537
Node decommissioning stage 2 was rolled out in a hurry so that we could make the holiday-release cutoff: #2953
These changes add the missing observability logic.
Main commits:
Screenshot of metrics:
