Retain traffic graph values

[chromatic]

Addresses #2523.

Let's see how this works in testing before committing to it (pun barely intended).

[patricklodder]

I was playing a bit with it while testing the pruning ui change (I compiled this on top), and I have no idea whether I'm testing it right, but attached is what I see when I slide the slider

not_sure2.mp4

[chromatic]

That looks like what I see too. I'm not sure if it's what @ReverseControl wants to see.

To me, this achieves the "don't throw away existing data" piece of the request, though the behavior I see seems to suggest this is retaining the data but not scaling/replotting it for the newly selected interval.

I'm not certain what the desired behavior is beyond "retain data", so I offer this PR to raise the deeper question.

[michilumin]

that doesn't look right-- Essentially you'd always want to be retaining "a day's worth of data or whatever's been sampled so far (max)" and just changing the 'viewport'... So basically on change of that slider you'd never want to call clearing of the sample vectors. Basically it has to be double buffered. @chromatic are you cool on me giving this a spin to see if I can make it do what @ReverseControl is asking? I think I get the gist...

[chromatic]

Please by all means, @michilumin! I have an idea what this might look like too, but won't have time to code for a couple of days.

[michilumin]

Ok, so a few things. The way I see this working, for it to operate like @ReverseControl would like: would require some big changes, and I'm going to attempt them, but there are some limitations.

Plotting the lines (800 'visual' samples with 400 shown -- nyquist to build a line, I think?) isn't a problem and can stay mostly the same.

However for the graph to retain data in a meaningful way (zoom in, zoom out, back again; re-plotting without clearing the samples) means that there will have to be a minimum regular sample rate (DATASAMPLERATE), which can properly fill the minimum slider time period (MINSLIDERTIME); and at that rate, it will have to store enough samples, in a circular buffer, to be able to fill the maximum slider time period (MAXSLIDERTIME).

In this case I'd store those samples in a circular buffer that is "1 day long, max * samplerate", so lets say the regular sampling was once a second; that'd be a circular buffer 86400 samples deep.

At MAXSLIDERTIME, we would look backwards at captured samples and average where one "visual sample" (of which the widget can display 800) is the average of (MAXSLIDERTIME x DATASAMPLERATE / MAXVISUALSAMPLES) so: (24h*1sps)/800; so at the maximum slider value of '1 day', we'd average 108 data samples into 1 visual sample.

So if the graph period was 6 hours, we'd average 27 data samples to equal 1 visual sample.

Unfortunately the 800 number that's already specified is a bit strange for values lower than an hour (4.5 data samples per visual sample) so working with that 800 number gets weird in this model, (life would be easier if it were 600 or 1200, assuming a 1 second sample rate) With 800, 1:1 on data to visual samples would be at 13.3 minutes. I don't think we want to get into subsampling and supersampling with a traffic widget. Talk about overengineering.

So maybe changing to 1200 visual samples used and 600 shown (2:1 to build a point on the graph) would be reasonable to do? In that case we could have 24h, 12h, 6h, 3h, 1h, 20m, and 10m as reasonable values for the graph's span.

I have to noodle on this math a bit more, but 800/400 doesn't really play nice with a graph that's showing fractions of a day/minutes/hours, if samples are taken at a reasonable rate.

The other issue on this change: The widget seems to only run (record) when it's visible. (and I think that's reasonable, from a resources point of view) - So if the user ever switched off of the traffic graph tab, it'd probably be reasonable to clear it and start over, and 'retain values' only makes sense while the graph is being viewed and the slider is being manipulated -- otherwise we'd have to have it recording, all the time, in the background. (Which maybe is reasonable? 86400 magnitudes isn't really that big of a memory requirement, and 1 event per second when it's just recording a value, may not impact anything?)

So -- thoughts?

[ReverseControl]

@chromatic Thanks for addressing this. I've been away for a bit, but I will take a look at this properly this week. From @patricklodder video clip it seems the slider is not doing anything to the graph which is not right as @michilumin pointed out, but it is not dropping data so that is an improvement.

The behavior that is useful to me as someone running a node is best exemplified by glasswire the firewall. In our case, we would limit the window to something reasonable, the sample rate should be a power of two per minute because cutting things in half is nice, and when we zoom in and zoom out we only show the data for that window. To michi's point, we need a good sample rate to show data points for small windows of time and we need about as many sample points to plot for longer periods of time. Given the longer time periods for bigger windows have more data points, i think what @michilumin suggested about averaging is a good way to compress that data to then plot it nicely.

Now in terms of features:

1. I think there should be a button somewhere to indicate the person running the node wants to record traffic volume even when that tab is not active. I certainly would want to keep an eye on that number on my nodes even if I am not on that tab..
2. There should be some reasonable default as to how many days of data to keep around.
3. Maybe give users to choice to store more. I think glasswire gives you the option to keep up to 30 days worth of traffic data, and there is one option to keep it for all time.

But, I think keeping the data around for a day or two is reasonable and replotting different time windows correctly should suffice for now. This alone would address my initial submission on this matter, I think.

[michilumin]

Maybe give users to choice to store more. I think glasswire gives you the option to keep up to 30 days worth of traffic data, and there is one option to keep it for all time.

Well - have to be careful here on how far this goes, at least initially. Are we talking persisting to disk now? Because that's a whole additional layer here of scope. 2 days in memory while running? Sure, okay, 172,800 integers isn't a big deal. But if you're talking about writing it to disk and loading it and continuing/archiving, (inferring, with the nod to Glasswire) - we're getting into a non-insignificant add. (What format/db do we use for disk write and retrieval? How often do disk writes happen? Load it all into memory on startup or just the samples displayed? How do we indicate borders between shutdowns on the graph?)

It's doable, sure, but a considerably longer pole and adds a lot of variables.

I think that at least for now restricting the scope to not throwing out data when the slider is moved, is probably a good limit for round one of this.

[chromatic]

I agree with Michi; let's keep the scope of this first round small. If users want more data over a longer time period, I'd rather make a separate issue to export this data via API/RPC and let clients handle persistence. That way we're not tied to any specific serialization implementation and can re-use the circular buffer.

[patricklodder]

I think that at least for now restricting the scope to not throwing out data when the slider is moved, is probably a good limit for round one of this.

Agree.

If users want more data over a longer time period, I'd rather make a separate issue to export this data via API/RPC and let clients handle persistence.

I'm personally using the rpc to export bandwidth usage for my nodes to prometheus and that works perfectly already.

[ReverseControl]

@michilumin Agreed. That was my last statement on my last post with the addition of re plotting correctly. @patricklodder solution works for now for folks who want to persist data for longer.

[michilumin]

So, this still needs some work, but maybe this is closer at least in concept to what @ReverseControl was looking for.

Sped up for demonstrative purposes (though honestly I kind of like how it looks with a faster sample rate, would be neat if that could be an option but -- that's something for later.)

output.mp4

[patricklodder]

@michilumin That looks pretty cool.

Do I see it right that you made it collect without having to be on the foreground? That's really neat and I think it is useful for when you see something happening in that graph and click it away to investigate (eg peers tab, main screen sync progress.)

👍

[michilumin]

It should always be collecting, yes - I'm going to test that behavior on a few platforms to make sure it's consistent, but it shouldn't behave differently.

the "aliasing" you see though is unfortunately a necessary evil since we only have so many lines to work with and more data points than lines - but honestly I think that's acceptable unless we were to moved to an antialiased (in graphics) line, which i feel is overkill.

[ReverseControl]

@michilumin Yes!! That's what I was looking for. :)

[michilumin]

cool.. I'll get it up on my repo here and do a pr into chromatic's ? not sure what best workflow would be at this stage since I kind of jumped in in the review stage.

[chromatic]

That would work for me; I'm happy to do whatever makes your life easier here @michilumin.

[michilumin]

Checked this on a few archs and seems to perform correctly. Can nuke the comments if you guys want, i just use them for my own organization and clarification.

[michilumin]

Now just need to find someone who's appropriate to review.. @chromatic ? 🧐

[patricklodder]

Now just need to find someone who's appropriate to review..

Not sure if I'm appropriate but I am reviewing.

[patricklodder]

First off, this looks like a good improvement. I've done review only, will test it in operation with a next iteration.

• Functional comment: I'm not sure about max aggregation instead of average
• Compiler throws some warnings about initialization order in the constructor code, should be an easy fix
• There's a bunch of size_t and int comparisons
• Couple of nits that save some cpu ticks and improve cohesion, not important in the grand scheme of things though.

All those can be found inline.

Re: comments. I think the comments themselves are informative. Initials and date may become confusing if someone changes something inside a block you're commenting on and as git tracks that for us anyway, I'd suggest to just remove initials/date but keep the comments, unless there is a "TODO" that you want to commit to.

[michilumin]

Reworked to use average and still display sufficiently. Also timing is more accurate; and cleaned up requested fixes.

Sped up, but example of the current rendering of the graph.

2022-05-12_3090.mp4

[michilumin]

Just noticed the windows line ending issue, causing the diff to look stupid. Is super late right now, will fix that tomorrow.

[patricklodder]

Tested this (on an M1) and although there's some "dancing" left, it's not as bad as it was. The only way to change that is by making the viewport fixed.

Other than Chromatic's comments and some nits, I'm good with this one.

[chromatic]

Code looks good to me. Because I have code in this PR, I won't offer formal approval, but it passes my muster.

[patricklodder]

@chromatic could you please squash commits 2,3,4? Other than that I think this is great.

(will compile + test once after squash)

[chromatic]

Rebased and squashed. I'm happy to find a way to amend this commit so we give @michilumin full credit for all the work here.

[patricklodder]

ACK. Tested on macOS Monterey (Apple M1)