Repository: fujiapple852/trippy (language: rust)

Issue:
Calculate and display Jitter
Trippy current calculates the `StDev` column as:

```rust
pub fn stddev_ms(&self) -> f64 {
    if self.total_recv > 1 {
        (self.m2 / (self.total_recv - 1) as f64).sqrt()
    } else {
        0_f64
    }
}
```

Where, for each successful `Probe` (for a given hop `ttl`), the `total_recv`, `mean` and `m2` values are updated as follows:

```rust
let dur = probe.duration();
let dur_ms = dur.as_secs_f64() * 1000_f64;
hop.total_recv += 1;
hop.mean += (dur_ms - hop.mean) / hop.total_recv as f64;
hop.m2 += (dur_ms - hop.mean) * (dur_ms - hop.mean);
```

We would like to add support for a Jitter columns similar to those provided by mtr (see https://networkengineering.stackexchange.com/questions/5288/what-is-interarrival-jitter-in-mtr)


Discussion / hints:
I am taking this issue on my existing fork.  I will be working towards a PR for review by EOW.
Here's a proposal for the feature after taking a look at the code. The calculation requires at least 2 round trips to be completed for each hop.  The millis Duration value from consecutive hops (HOPn - HOPn-1) results in jitter duration.
Calc: Jitter = RTTn - RTTn-1
RTT = Round trip time
1) Add --jitter -j option for the command line.  Starts up in jitter display mode.
2) Headings will have a "J" prepended in the display.
3) Add values to any reporting like csv.
4) Might need jitter mode to Mode enum tui-jitter.
5) Stretch - When tui display is running pressing "j" will switch to jitter mode.  This comes from having used mtr in the passed.

Am I overthinking it should I scale it back?
@trkelly23 thanks for picking this up!

> Add --jitter -j option for the command line. Starts up in jitter display mode.
> Headings will have a "J" prepended in the display.

Which headings would be prefixed?  I had imagined these jitter related values as _additional_ columns in the hops table, is that what you are proposing?  

A mockup of what I envisioned:

```
╭Hops───────────────────────────────────────────────────────────────────────────────────────────╮
│#   Host          Loss% Snt   Recv  Last  Avg   Best  Wrst  StDev  Jttr  Javg  jmax  Jint  Sts │
│2   10.76.194.68  0.0%  118   118   0.8   1.3   0.5   22.0  2.7    xxx   xxx   xxx   xxx   🟢  │
```
Is the proposal that these columns would be calculated and/or shown only if the `--jitter` (`-j`) flag is specified?  While I think that is fine, what do you think about doing https://github.com/fujiapple852/trippy/issues/757 first or along with this change so that the selection of columns to show can be more flexible?

> Add values to any reporting like csv.

Yes that makes sense, we should add to all report modes.  

> Might need jitter mode to Mode enum tui-jitter.

Right, similar to `--tui-as-mode` we may want a `--tui-jitter-mode` or similar to control how jitter is displayed in the Tui.  It could be as simple as a bool to turn jitter display on/off or an enum to give finer control of which columns are shown.  Just to be clear, I don't think a new `--mode tui-jitter` is needed here however.  

> When tui display is running pressing "j" will switch to jitter mode. This comes from having used mtr in the passed.

Yes that would be useful.  It shouldn't be to difficult to add as there are several other such things in the Tui already, such as pressing `z` to toggle the AS display mode.  I can always help you with this part if needed.


> The calculation requires at least 2 round trips to be completed for each hop. The millis Duration value from consecutive hops (HOPn - HOPn-1) results in jitter duration.
Calc: Jitter = RTTn - RTTn-1
RTT = Round trip time

I just had a look in `mtr` and, whilst we don't need to restrict ourselves to doing things the same way, it does serve as a useful reference point.  

It allows specifying these additional (non-default) columns:

```
Jttr: Current Jitter
Javg: Jitter Mean/Avg.
Jmax: Worst Jitter
Jint: Interarrival Jitter
```

It would be useful to look at how these are calculated and aim to emulate the logic here.  From a quick glance they seem more complex than simply `RTTn - RTTn-1` though I confess statistics is not my area of expertise!
I have been working on the calculations for Jitter and wrote up a little test file with tests to validate.  After checking mtr code and description of jitter per the spec RFC-3550 I was able to create a spreadsheet to calculate the values and code to calculate.  The Jitter struct is for test purposes and these fields would go into Hop.  Note the cumulative average calculation I'm using may have uncovered a bug in the hop.mean calculation.  

Please, take a look at the calculation approach.

[jitter.txt](https://github.com/fujiapple852/trippy/files/13722315/jitter.txt)
The above file changed to .txt .rs not allowed.


> Note the cumulative average calculation I'm using may have uncovered a bug in the hop.mean calculation

Can you give some more details on the potential bug?

Aside from the fix I did in commit 4e90da79f7af2511dd03f7bb8377b2a8953f3cac (April 2022) the logic seems to have been the same since the very beginning.  I don't recall what prompted me to fix that, I _suspect_ I was doing a side-by-side comparison with MTR and was correcting for a divergence between the output.

I can't remember exactly, but I believe this may have been my translation of [Welford's online algorithm](https://en.wikipedia.org/wiki/Algorithms_for_calculating_variance#Welford's_online_algorithm) for computing variance on a stream of data in a single pass (note the use of `m2` in that algorithm and my original code).

> Please, take a look at the calculation approach

I've had a look but I may not be understand correctly.  Parsing at the code and paraphrasing, it seems the calculation you have are:

Javg:

```
Javg = ((Javg * total_recv) + dur_ms) / total_recv
```

Jint:
```
Jint = abs(dur_ms - Javg)
```

Jmax:

```
Jmax = max(Jmax, dur_ms)
```

The last one seems more like an overall max/worst calculation, rather than a mac/worst _jitter_ calculation?

I don't see the basic `Jttr` calculation, unless that is what `value` is supported represent?  That seems to be simply using `dur_ms` which doesn't seem right if so?

How do these calculations compare to the logic in MTR?  Maybe best to impl these on a branch and do a side-by-side comparison?
I put a PR up after taking a look at Welford's calculation and a few stats sites.  I see now the avg can be calculated if you have the total number in the dataset,  the current avg & the new value.  Nice much less processing and overhead.
I also verified the Jitter average (javg) is cumulative according to mtr.
As you said best to move the conversation to a PR.

Thanks for all the feedback and happy holidays! 🎅 




Thanks and happy holidays to you as well.

I haven't had a chance to look at the code yet but let me know if you are doing any other calculations that are not "streaming friendly" and I can take a look as I still remember a lot of those I learned in school of which mean is one (the one you already found).

Fix patch:
diff --git a/src/backend/trace.rs b/src/backend/trace.rs
index 619ffbafe..c7d735371 100644
--- a/src/backend/trace.rs
+++ b/src/backend/trace.rs
@@ -138,6 +138,14 @@ pub struct Hop {
     best: Option<Duration>,
     /// The worst round trip time for this hop across all rounds.
     worst: Option<Duration>,
+    /// The current jitter i.e. round-trip difference with the last round-trip.
+    jitter: Option<Duration>,
+    /// The average jitter time for all probes at this hop.
+    javg: f64,
+    /// The worst round-trip jitter time for all probes at this hop.
+    jmax: Option<Duration>,
+    /// The smoothed jitter value for all probes at this hop.
+    jinta: f64,
     /// The history of round trip times across the last N rounds.
     samples: Vec<Duration>,
     /// The ICMP extensions for this hop.
@@ -219,6 +227,25 @@ impl Hop {
         }
     }
 
+    /// The duration of the jitter probe observed.
+    pub fn jitter_ms(&self) -> Option<f64> {
+        self.jitter.map(|j| j.as_secs_f64() * 1000_f64)
+    }
+
+    /// The duration of the jworst probe observed.
+    pub fn jmax_ms(&self) -> Option<f64> {
+        self.jmax.map(|x| x.as_secs_f64() * 1000_f64)
+    }
+
+    /// The jitter average duration of all probes.
+    pub fn javg_ms(&self) -> f64 {
+        self.javg
+    }
+    /// The jitter interval of all probes.
+    pub fn jinta(&self) -> f64 {
+        self.jinta
+    }
+
     /// The last N samples.
     pub fn samples(&self) -> &[Duration] {
         &self.samples
@@ -240,6 +267,10 @@ impl Default for Hop {
             last: None,
             best: None,
             worst: None,
+            jitter: None,
+            javg: 0f64,
+            jmax: None,
+            jinta: 0f64,
             mean: 0f64,
             m2: 0f64,
             samples: Vec::default(),
@@ -336,6 +367,17 @@ impl TraceData {
                 let dur = probe.duration();
                 let dur_ms = dur.as_secs_f64() * 1000_f64;
                 hop.total_time += dur;
+                // Before last is set use it to calc jitter
+                let last_ms = hop.last_ms().unwrap_or_default();
+                let jitter_ms = (last_ms - dur_ms).abs();
+                let jitter_dur = Duration::from_secs_f64(jitter_ms / 1000_f64);
+                hop.jitter = hop.last.and(Some(jitter_dur));
+                hop.javg += (jitter_ms - hop.javg) / hop.total_recv as f64;
+                // algorithm is from rfc1889, A.8 or rfc3550
+                hop.jinta += jitter_ms - ((hop.jinta + 8.0) / 16.0);
+                hop.jmax = hop
+                    .jmax
+                    .map_or(Some(jitter_dur), |d| Some(d.max(jitter_dur)));
                 hop.last = Some(dur);
                 hop.samples.insert(0, dur);
                 hop.best = hop.best.map_or(Some(dur), |d| Some(d.min(dur)));
diff --git a/src/config/columns.rs b/src/config/columns.rs
index ac69c9a35..45b6a4e4f 100644
--- a/src/config/columns.rs
+++ b/src/config/columns.rs
@@ -71,6 +71,14 @@ pub enum TuiColumn {
     StdDev,
     /// The status of a hop.
     Status,
+    /// The current jitter i.e. round-trip difference with the last round-trip.
+    Jitter,
+    /// The average jitter time for all probes at this hop.
+    Javg,
+    /// The worst round-trip jitter time for all probes at this hop.
+    Jmax,
+    /// The smoothed jitter value for all probes at this hop.
+    Jinta,
 }
 
 impl TryFrom<char> for TuiColumn {
@@ -89,6 +97,10 @@ impl TryFrom<char> for TuiColumn {
             'w' => Ok(Self::Worst),
             'd' => Ok(Self::StdDev),
             't' => Ok(Self::Status),
+            'j' => Ok(Self::Jitter),
+            'g' => Ok(Self::Javg),
+            'x' => Ok(Self::Jmax),
+            'i' => Ok(Self::Jinta),
             c => Err(anyhow!(format!("unknown column code: {c}"))),
         }
     }
@@ -108,6 +120,10 @@ impl Display for TuiColumn {
             Self::Worst => write!(f, "w"),
             Self::StdDev => write!(f, "d"),
             Self::Status => write!(f, "t"),
+            Self::Jitter => write!(f, "j"),
+            Self::Javg => write!(f, "g"),
+            Self::Jmax => write!(f, "x"),
+            Self::Jinta => write!(f, "i"),
         }
     }
 }
@@ -134,7 +150,7 @@ mod tests {
     }
 
     ///Negative test for invalid characters
-    #[test_case('x' ; "invalid x")]
+    #[test_case('k' ; "invalid k")]
     #[test_case('z' ; "invalid z")]
     fn test_try_invalid_char_for_tui_column(c: char) {
         // Negative test for an unknown character
diff --git a/src/frontend/columns.rs b/src/frontend/columns.rs
index ea1e81e35..b33c2d6f9 100644
--- a/src/frontend/columns.rs
+++ b/src/frontend/columns.rs
@@ -79,6 +79,14 @@ pub enum Column {
     StdDev,
     /// The status of a hop.
     Status,
+    /// The current jitter i.e. round-trip difference with the last round-trip.
+    Jitter,
+    /// The average jitter time for all probes at this hop.
+    Javg,
+    /// The worst round-trip jitter time for all probes at this hop.
+    Jmax,
+    /// The smoothed jitter value for all probes at this hop.
+    Jinta,
 }
 
 impl From<Column> for char {
@@ -95,6 +103,10 @@ impl From<Column> for char {
             Column::Worst => 'w',
             Column::StdDev => 'd',
             Column::Status => 't',
+            Column::Jitter => 'j',
+            Column::Javg => 'g',
+            Column::Jmax => 'x',
+            Column::Jinta => 'i',
         }
     }
 }
@@ -113,6 +125,10 @@ impl From<TuiColumn> for Column {
             TuiColumn::Worst => Self::Worst,
             TuiColumn::StdDev => Self::StdDev,
             TuiColumn::Status => Self::Status,
+            TuiColumn::Jitter => Self::Jitter,
+            TuiColumn::Javg => Self::Javg,
+            TuiColumn::Jmax => Self::Jmax,
+            TuiColumn::Jinta => Self::Jinta,
         }
     }
 }
@@ -131,6 +147,10 @@ impl Display for Column {
             Self::Worst => write!(f, "Wrst"),
             Self::StdDev => write!(f, "StDev"),
             Self::Status => write!(f, "Sts"),
+            Self::Jitter => write!(f, "Jttr"),
+            Self::Javg => write!(f, "Javg"),
+            Self::Jmax => write!(f, "Jmax"),
+            Self::Jinta => write!(f, "Jint"),
         }
     }
 }
@@ -151,6 +171,10 @@ impl Column {
             Self::Worst => ColumnWidth::Fixed(7),
             Self::StdDev => ColumnWidth::Fixed(8),
             Self::Status => ColumnWidth::Fixed(7),
+            Self::Jitter => ColumnWidth::Fixed(7),
+            Self::Javg => ColumnWidth::Fixed(7),
+            Self::Jmax => ColumnWidth::Fixed(7),
+            Self::Jinta => ColumnWidth::Fixed(8),
         }
     }
 }
diff --git a/src/frontend/render/table.rs b/src/frontend/render/table.rs
index 425e87bc2..af53576b5 100644
--- a/src/frontend/render/table.rs
+++ b/src/frontend/render/table.rs
@@ -17,7 +17,7 @@ use trippy::tracing::{Extension, Extensions, MplsLabelStackMember, UnknownExtens
 
 /// Render the table of data about the hops.
 ///
-/// For each hop, we show:
+/// For each hop, we show by default:
 ///
 /// - The time-to-live (indexed from 1) at this hop (`#`)
 /// - The host(s) reported at this hop (`Host`)
@@ -30,6 +30,13 @@ use trippy::tracing::{Extension, Extensions, MplsLabelStackMember, UnknownExtens
 /// - The worst round-trip time for all probes at this hop (`Wrst`)
 /// - The standard deviation round-trip time for all probes at this hop (`StDev`)
 /// - The status of this hop (`Sts`)
+///
+/// Optional columns that can be added:
+///
+/// - The current jitter i.e. round-trip difference with the last round-trip ('Jttr')
+/// - The average jitter time for all probes at this hop ('Javg')
+/// - The worst round-trip jitter time for all probes at this hop ('Jmax')
+/// - The smoothed jitter value for all probes at this hop ('Jinta')
 pub fn render(f: &mut Frame<'_>, app: &mut TuiApp, rect: Rect) {
     let config = &app.tui_config;
     let widths = config.tui_columns.constraints(rect);
@@ -132,7 +139,7 @@ fn new_cell(
 ) -> Cell<'static> {
     let is_target = app.tracer_data().is_target(hop, app.selected_flow);
     match column {
-        Column::Ttl => render_ttl_cell(hop),
+        Column::Ttl => render_usize_cell(hop.ttl().into()),
         Column::Host => {
             let (host_cell, _) = if is_selected_hop && app.show_hop_details {
                 render_hostname_with_details(app, hop, dns, geoip_lookup, config)
@@ -142,32 +149,29 @@ fn new_cell(
             host_cell
         }
         Column::LossPct => render_loss_pct_cell(hop),
-        Column::Sent => render_total_sent_cell(hop),
-        Column::Received => render_total_recv_cell(hop),
-        Column::Last => render_last_cell(hop),
+        Column::Sent => render_usize_cell(hop.total_sent()),
+        Column::Received => render_usize_cell(hop.total_recv()),
+        Column::Last => render_float_cell(hop.last_ms(), 1),
         Column::Average => render_avg_cell(hop),
-        Column::Best => render_best_cell(hop),
-        Column::Worst => render_worst_cell(hop),
+        Column::Best => render_float_cell(hop.best_ms(), 1),
+        Column::Worst => render_float_cell(hop.worst_ms(), 1),
         Column::StdDev => render_stddev_cell(hop),
         Column::Status => render_status_cell(hop, is_target),
+        Column::Jitter => render_float_cell(hop.jitter_ms(), 1),
+        Column::Javg => render_float_cell(Some(hop.javg_ms()), 1),
+        Column::Jmax => render_float_cell(hop.jmax_ms(), 1),
+        Column::Jinta => render_float_cell(Some(hop.jinta()), 1),
     }
 }
-fn render_ttl_cell(hop: &Hop) -> Cell<'static> {
-    Cell::from(format!("{}", hop.ttl()))
+
+fn render_usize_cell(value: usize) -> Cell<'static> {
+    Cell::from(format!("{value}"))
 }
 
 fn render_loss_pct_cell(hop: &Hop) -> Cell<'static> {
     Cell::from(format!("{:.1}%", hop.loss_pct()))
 }
 
-fn render_total_sent_cell(hop: &Hop) -> Cell<'static> {
-    Cell::from(format!("{}", hop.total_sent()))
-}
-
-fn render_total_recv_cell(hop: &Hop) -> Cell<'static> {
-    Cell::from(format!("{}", hop.total_recv()))
-}
-
 fn render_avg_cell(hop: &Hop) -> Cell<'static> {
     Cell::from(if hop.total_recv() > 0 {
         format!("{:.1}", hop.avg_ms())
@@ -176,30 +180,6 @@ fn render_avg_cell(hop: &Hop) -> Cell<'static> {
     })
 }
 
-fn render_last_cell(hop: &Hop) -> Cell<'static> {
-    Cell::from(
-        hop.last_ms()
-            .map(|last| format!("{last:.1}"))
-            .unwrap_or_default(),
-    )
-}
-
-fn render_best_cell(hop: &Hop) -> Cell<'static> {
-    Cell::from(
-        hop.best_ms()
-            .map(|best| format!("{best:.1}"))
-            .unwrap_or_default(),
-    )
-}
-
-fn render_worst_cell(hop: &Hop) -> Cell<'static> {
-    Cell::from(
-        hop.worst_ms()
-            .map(|worst| format!("{worst:.1}"))
-            .unwrap_or_default(),
-    )
-}
-
 fn render_stddev_cell(hop: &Hop) -> Cell<'static> {
     Cell::from(if hop.total_recv() > 1 {
         format!("{:.1}", hop.stddev_ms())
@@ -208,6 +188,10 @@ fn render_stddev_cell(hop: &Hop) -> Cell<'static> {
     })
 }
 
+fn render_float_cell(value: Option<f64>, places: usize) -> Cell<'static> {
+    Cell::from(value.map(|v| format!("{v:.places$}")).unwrap_or_default())
+}
+
 fn render_status_cell(hop: &Hop, is_target: bool) -> Cell<'static> {
     let lost = hop.total_sent() - hop.total_recv();
     Cell::from(match (lost, is_target) {
diff --git a/src/report/types.rs b/src/report/types.rs
index 7cde0733e..0cb1a060d 100644
--- a/src/report/types.rs
+++ b/src/report/types.rs
@@ -35,6 +35,14 @@ pub struct Hop {
     pub worst: f64,
     #[serde(serialize_with = "fixed_width")]
     pub stddev: f64,
+    #[serde(serialize_with = "fixed_width")]
+    pub jitter: f64,
+    #[serde(serialize_with = "fixed_width")]
+    pub javg: f64,
+    #[serde(serialize_with = "fixed_width")]
+    pub jmax: f64,
+    #[serde(serialize_with = "fixed_width")]
+    pub jinta: f64,
 }
 
 impl<R: Resolver> From<(&backend::trace::Hop, &R)> for Hop {
@@ -53,6 +61,10 @@ impl<R: Resolver> From<(&backend::trace::Hop, &R)> for Hop {
             best: value.best_ms().unwrap_or_default(),
             worst: value.worst_ms().unwrap_or_default(),
             stddev: value.stddev_ms(),
+            jitter: value.jitter_ms().unwrap_or_default(),
+            javg: value.javg_ms(),
+            jmax: value.jmax_ms().unwrap_or_default(),
+            jinta: value.jinta(),
         }
     }
 }


Test patch: