feat display jitter #882
feat display jitter #882
Conversation
trkelly23
force-pushed
the
feat-display-jitter
branch
from
35ecd87 to
0955ae8
Compare
| let last_ms = hop.last_ms().map_or(0f64, |d|d); | ||
| let jitter_ms = (last_ms - dur_ms).abs(); | ||
| let jitter_dur = Duration::from_millis(jitter_ms as u64); | ||
| hop.jitter = Some(jitter_dur); |
There was a problem hiding this comment.
This is set to be Some unconditionally, I guess we should only set this if hop.last is Some?
|
@trkelly23 I've added a few minor review comments on the impl but aside from that the logic of all four calculations ( |
Yes, I think we're now going to have to solve the general issue of table column sizing when we have a variable number of columns with varying minimum size requirements. The switch here to use
I think this is fine for now. There are lots of other columns I would like to add that would not be shown be default (for example, estimate geographic distance between hops when both GeoIps are known). As we discussed before, in the future we may want to add a dialog for dynamic column selection. |
I have run a many side by side tests the best comparison method was to add the jitter fields to trippy JSON output and run both. A set of results run 1/1/2024. |
trkelly23
force-pushed
the
feat-display-jitter
branch
from
0955ae8 to
0d75c2e
Compare
trkelly23
force-pushed
the
feat-display-jitter
branch
4 times, most recently
from
1bed4bb to
f113f8a
Compare
Merge in of fujiapple852#925 improvement on column display. feat display jitter feat display jitter(WIP) Includes possible test approach using JSON to mimic trace data feat(tui): improve column width layout logic (WIP)
trkelly23
force-pushed
the
feat-display-jitter
branch
from
f113f8a to
d026669
Compare
| jinta, | ||
| } | ||
| } | ||
| //TODO: Create combined struct ImportProbe Vec<ImportProbe> & HopResults. |
There was a problem hiding this comment.
I think that is a good idea. Can we use yaml rather than json as well?
There was a problem hiding this comment.
After looking at the TUN test code I had the same exact thought, make all test input files the same type.
Consider it done!
| @@ -52,6 +52,7 @@ tracing-subscriber = { version = "0.3.18", default-features = false, features = | |||
| tracing-chrome = "0.7.1" | |||
| petgraph = "0.6.4" | |||
| csv = "1.3.0" | |||
| iso8601-timestamp = "0.2.16" | |||
There was a problem hiding this comment.
Can we avoid needing this dep? Can humantime be used instead? Note it could be in [dev-dependancies] as well.
There was a problem hiding this comment.
I see input the difference (RTTx - RTTx-1) - I can easily move to this type of model.
Yes, it should only be needed as a [dev-dependancies].
| #[test_case("base_line.json", &HopResults::new(2,2,700,300,700,0.0,0.0,400,350.0,400,680.28))] | ||
| fn test_probe_file(json_file: &str, expected: &HopResults) { | ||
| let mut json = String::new(); | ||
| let mut dir_json_file = "./tests/data/".to_owned(); |
There was a problem hiding this comment.
Can we use include_str!() to read these at compile time? See
Lines 31 to 36 in 55641c6
|
Superseded by #937 |
The branch is currently a WIP to get feedback on Jitter calculations and display.
Know issues:
The display will overflow if all the columns are displayed; though as an interim fix, the Constraint::Min is used.
Jitter columns will only show using tui-custom-columns
Jitter columns display in json & tui mode's only. Json was added for saving test results for comparison to mtr.
Closes #39