Display weighted average of bandwidth #77

zhangxp1998 · 2020-01-05T20:09:00Z

The idea of exponential decay is used. When displaying bandwidth, it takes an weighted average of all bandwidth in the past. The bandwidth of last 1 second gets weight 0.5, the bandwidth of last 2 seconds gets weight 0.25, etc.

This change should smoothen the be bandwidth curve somewhat, while giving recent bandwidth data more weight.

zhangxp1998 · 2020-01-05T20:21:18Z

@imsnif Looks like some UI tests have failed, possibly because I changed UI. But I'm not sure how to interpret the snapshots. Would you give some guidance?

imsnif · 2020-01-05T20:29:27Z

Sure - happy to see you're picking this up. I like the direction this is going. Fair warning, it might take me a few days to give this a thorough review. Meanwhile so you're not stuck though:

The tests use insta in order to perform snapshot testing: https://github.com/mitsuhiko/insta
If you run them locally, they will compare the state of the app in various situations against pre-recorded snapshots. If the comparison fails, they will create temporary files with the snapshots they received. If you override the existing snapshots with these files, the tests will pass.

You can automate this process and make reviews easier if you use insta to review. You need to install it locally and then you'd be able to cargo insta review and see the diff.

Here, I imagine the difference would mostly be in the bandwidth values, as their calculation has now changed.

I would also like to add tests that specifically test the new bandwidth calculation (eg. run traffic for longer and see that the calculation is correct).

Makes sense?

imsnif · 2020-01-05T20:32:12Z

By "state of the app" I mean the state of the UI. What you see on the screen. If you cat the first snapshot of a test, you'll see the initial UI. The snapshot after it will be the changes to the screen at each update interval (1s currently).

chobeat · 2020-01-06T08:57:17Z

src/display/ui_state.rs

@@ -30,6 +39,24 @@ impl Bandwidth for ConnectionData {
    fn get_total_bytes_downloaded(&self) -> u128 {
        self.total_bytes_downloaded
    }
+    fn get_avg_bytes_uploaded(&self) -> u128 {


this code is repeated 4 times, does it make sense to refactor it to split the averaging logic from the access to the field?

make sense, let me refactor that

imsnif · 2020-01-06T16:51:10Z

Hey @zhangxp1998 - this is great!

Before going into the implementation, I'd like to first play around with the behaviour.

Currently, the information on the UI still flickers in and out without a lot of chance at reading it unless it's a sustained connection.

Looking (admittedly very briefly!) through the code, we seem to be saving just 1 state back, is that correct? Do you think we can make it save an arbitrary amount of states back? Like, try for 10, see if the UI feels better and play with it from there?

zhangxp1998 · 2020-01-06T16:54:27Z

Hey @zhangxp1998 - this is great!

Before going into the implementation, I'd like to first play around with the behaviour.

Currently, the information on the UI still flickers in and out without a lot of chance at reading it unless it's a sustained connection.

Looking (admittedly very briefly!) through the code, we seem to be saving just 1 state back, is that correct? Do you think we can make it save an arbitrary amount of states back? Like, try for 10, see if the UI feels better and play with it from there?

Yes I'm only tracking 1 state back. But that state contain information about traffic from beginning of time(exponential decaying weight). So it's different from just averaging 2 seconds of traffic.

imsnif · 2020-01-06T17:06:42Z

Yes I'm only tracking 1 state back. But that state contain information about traffic from beginning of time(exponential decaying weight). So it's different from just averaging 2 seconds of traffic.

Ah, cool. I think it would be great if the connections appear on the screen longer.

As in, if we're keeping info about 10 states back, that if sometime during those 10 seconds we had traffic, it would appear on screen. What do you say?

zhangxp1998 · 2020-01-06T17:17:49Z

Yes I'm only tracking 1 state back. But that state contain information about traffic from beginning of time(exponential decaying weight). So it's different from just averaging 2 seconds of traffic.

Ah, cool. I think it would be great if the connections appear on the screen longer.

As in, if we're keeping info about 10 states back, that if sometime during those 10 seconds we had traffic, it would appear on screen. What do you say?

I think it's a good idea. 10 maybe too long though, maybe make it 3-5?

imsnif · 2020-01-06T17:38:37Z

Sure. Let's see how it behaves and play around with it until it feels good.

zhangxp1998 · 2020-01-07T01:57:05Z

This PR and PR82 probably conflicts with each other... Which one should we work on first?

imsnif · 2020-01-07T07:25:51Z

This PR and PR82 probably conflicts with each other... Which one should we work on first?

This one. I'm sure we can handle the merge once it's done.

imsnif · 2020-01-12T12:59:23Z

Hey @zhangxp1998 - I pushed a commit with a suggestion for storing the previous states and displaying their average in the UI. I didn't factor in the bandwidth decay - was hoping you could include it, because I think you had a specific vision for it and I liked your direction.

If you run this branch, you'll see that the tool is much less erratic, and IMO displays the bandwidth in a more comfortable and less "blinky" way than what's currently on master. We could also play around with the RECALL_LENGTH as we discussed above. Would love to hear your thoughts.

(if you don't like this direction, feel free to revert my commit... tbh, I just felt like coding and wanted to lend a hand to this feature :) )

zhangxp1998

Cool! I like this! Just 1 knitpick

zhangxp1998 · 2020-01-12T15:05:12Z

src/display/ui_state.rs

 }

 #[derive(Default)]
 pub struct NetworkData {
    pub total_bytes_downloaded: u128,
    pub total_bytes_uploaded: u128,
+    pub prev_total_bytes_downloaded: u128,


Now these fields are not used anymore, perhaps we should remove them?

Ah yeah - for sure. Didn't want to cause too much of a mess in case you were in the middle of something.

I saw you've removed this already though, so thanks :)

imsnif · 2020-01-12T15:54:40Z

I don't mind fixing all the tests so this can pass - but wanted to know if you'd like to incorporate the bandwidth decay before? I'm also happy with going ahead like this and adding the bandwidth decay later if we want to - what do you think?

zhangxp1998 · 2020-01-12T15:55:54Z

I don't mind fixing all the tests so this can pass - but wanted to know if you'd like to incorporate the bandwidth decay before? I'm also happy with going ahead like this and adding the bandwidth decay later if we want to - what do you think?

I couldn't think of any way to incorporate bandwidth decay into this easily. So let's add that later

zhangxp1998 · 2020-01-12T16:00:18Z

The computed bandwidth seems incorrect: When a process engages in a burst of traffic, the displayed bandwidth will be much smaller than the instantaneous bandwidth. Is this okay though...

imsnif · 2020-01-12T16:06:09Z

The computed bandwidth seems incorrect: When a process engages in a burst of traffic, the displayed bandwidth will be much smaller than the instantaneous bandwidth. Is this okay though...

Yeah, this is the trade-off. We will now show the average of the last 5 seconds. If the burst lasts less than that, we'll swallow it. Do you feel this hurts the functionality too much?

zhangxp1998 · 2020-01-12T16:17:48Z

The computed bandwidth seems incorrect: When a process engages in a burst of traffic, the displayed bandwidth will be much smaller than the instantaneous bandwidth. Is this okay though...

Yeah, this is the trade-off. We will now show the average of the last 5 seconds. If the burst lasts less than that, we'll swallow it. Do you feel this hurts the functionality too much?

I think this is a reasonable trade off, but we should probably document this somewhere to not confuse ourselves/users in the future.

zhangxp1998 requested a review from imsnif January 5, 2020 20:19

chobeat reviewed Jan 6, 2020

View reviewed changes

zhangxp1998 force-pushed the average branch from aa0a676 to e202799 Compare January 6, 2020 13:31

zhangxp1998 added 2 commits January 9, 2020 20:02

Display weighted average of bandwidth

2c7dd50

Update UI snapshots for new bandwidth calculation

36d25e7

zhangxp1998 force-pushed the average branch from e202799 to 36d25e7 Compare January 10, 2020 01:16

suggestion for previous state storage and calculation

f6d1fc6

zhangxp1998 commented Jan 12, 2020

View reviewed changes

Fix formatting issue, remove deadcode

414dd76

zhangxp1998 force-pushed the average branch from f337fa2 to 414dd76 Compare January 12, 2020 15:11

test(snapshots): update to new formula

358e3f0

zhangxp1998 merged commit b843e18 into master Jan 12, 2020

zhangxp1998 deleted the average branch January 12, 2020 16:35

imsnif mentioned this pull request Jan 13, 2020

fix(ui-state): do not overcalculate bandwidth and connections #114

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display weighted average of bandwidth #77

Display weighted average of bandwidth #77

zhangxp1998 commented Jan 5, 2020

zhangxp1998 commented Jan 5, 2020

imsnif commented Jan 5, 2020

imsnif commented Jan 5, 2020

chobeat Jan 6, 2020

zhangxp1998 Jan 6, 2020

imsnif commented Jan 6, 2020

zhangxp1998 commented Jan 6, 2020

imsnif commented Jan 6, 2020 •

edited

Loading

zhangxp1998 commented Jan 6, 2020

imsnif commented Jan 6, 2020

zhangxp1998 commented Jan 7, 2020

imsnif commented Jan 7, 2020

imsnif commented Jan 12, 2020

zhangxp1998 left a comment

zhangxp1998 Jan 12, 2020

imsnif Jan 12, 2020

imsnif commented Jan 12, 2020

zhangxp1998 commented Jan 12, 2020

zhangxp1998 commented Jan 12, 2020

imsnif commented Jan 12, 2020

zhangxp1998 commented Jan 12, 2020

Display weighted average of bandwidth #77

Display weighted average of bandwidth #77

Conversation

zhangxp1998 commented Jan 5, 2020

zhangxp1998 commented Jan 5, 2020

imsnif commented Jan 5, 2020

imsnif commented Jan 5, 2020

chobeat Jan 6, 2020

Choose a reason for hiding this comment

zhangxp1998 Jan 6, 2020

Choose a reason for hiding this comment

imsnif commented Jan 6, 2020

zhangxp1998 commented Jan 6, 2020

imsnif commented Jan 6, 2020 • edited Loading

zhangxp1998 commented Jan 6, 2020

imsnif commented Jan 6, 2020

zhangxp1998 commented Jan 7, 2020

imsnif commented Jan 7, 2020

imsnif commented Jan 12, 2020

zhangxp1998 left a comment

Choose a reason for hiding this comment

zhangxp1998 Jan 12, 2020

Choose a reason for hiding this comment

imsnif Jan 12, 2020

Choose a reason for hiding this comment

imsnif commented Jan 12, 2020

zhangxp1998 commented Jan 12, 2020

zhangxp1998 commented Jan 12, 2020

imsnif commented Jan 12, 2020

zhangxp1998 commented Jan 12, 2020

imsnif commented Jan 6, 2020 •

edited

Loading