Issue 163 elapsed time #167

Eosis · 2020-04-26T13:30:09Z

I found this project recently and think its great. I have done quite a bit of packet-snooping work in C and C++ and think it's great to see a utility like this in Rust. 😁

I've started looking into #163 as a first issue to learn more about the program and practice some Rust (new rustacean here). I think I have an implementation working, but it still needs a bit more work to get the tests passing and some cleanup.

I'd welcome some feedback and maybe some advice about getting these failing tests passing... 😅

failures:
    tests::cases::ui::basic_only_addresses
    tests::cases::ui::basic_only_connections
    tests::cases::ui::basic_only_processes
    tests::cases::ui::basic_processes_with_dns_queries
    tests::cases::ui::basic_startup
    tests::cases::ui::bi_directional_traffic
    tests::cases::ui::layout_full_width_under_30_height
    tests::cases::ui::layout_under_120_width_full_height
    tests::cases::ui::layout_under_120_width_under_30_height
    tests::cases::ui::multiple_connections_from_remote_address
    tests::cases::ui::multiple_packets_of_traffic_from_different_connections
    tests::cases::ui::multiple_packets_of_traffic_from_single_connection
    tests::cases::ui::multiple_processes_with_multiple_connections
    tests::cases::ui::no_resolve_mode
    tests::cases::ui::one_packet_of_traffic
    tests::cases::ui::one_process_with_multiple_connections
    tests::cases::ui::pause_by_space
    tests::cases::ui::sustained_traffic_from_multiple_processes
    tests::cases::ui::sustained_traffic_from_multiple_processes_bi_directional
    tests::cases::ui::sustained_traffic_from_multiple_processes_bi_directional_total
    tests::cases::ui::sustained_traffic_from_multiple_processes_total
    tests::cases::ui::sustained_traffic_from_one_process
    tests::cases::ui::sustained_traffic_from_one_process_total
    tests::cases::ui::traffic_with_host_names
    tests::cases::ui::traffic_with_winch_event
    tests::cases::ui::truncate_long_hostnames
    tests::cases::ui::two_packets_only_addresses
    tests::cases::ui::two_packets_only_connections
    tests::cases::ui::two_packets_only_processes
    tests::cases::ui::two_windows_split_horizontally
    tests::cases::ui::two_windows_split_vertically

This elapsed time pauses during pause sections and restarts on resumption. Ongoing: - Consider a RwLock instead of a Mutex for the elapsed time and cumulative time values, as these are only written by the UI thread and read elsewhere. - Combine the cumulative time and start times into a struct for clarity. - Separate out the elapsed time from the Bandwidth, as they are separate considerations, leading to altering the rendering of the header to write bandwidth and elapsed time separately.

- Rename Bandwidth to HeaderDetails to indicate the added information it now contains. - Split the rendering function for the header into its constituent parts

A RwLock should be more performant.

The bandwidth print has a leading space, so this should have a trailing space.

imsnif · 2020-04-26T19:34:50Z

Hey @Eosis, I'm glad you like bandwhich. the screenshots look great!

About the tests, we use cargo insta for our snapshot testing. I should really write some basic instructions about it in the contributing guidelines - my apologies!

In a nutshell, what it does is runs fake traffic through bandwhich in different scenarios and takes snapshots of the UI, comparing them to the existing snapshots. These snapshots are part of the repository as well in the "snapshots" folder by the relevant tests.

When the tests fail, they create a new file with a .new suffix with the new snapshot. The way to make the tests pass is to override the old snapshot with the new one. Insta also has a cargo plugin that does this (described in their readme). Once you install it, you can do a cargo insta review after the tests fail, see the diff of the old/new snapshots and decide which of them you'd like to keep.

I hope that makes sense and answers your question?

TheLostLambda · 2020-04-26T19:41:04Z

Hi @Eosis !

I've taken a quick look at things and they are looking good! Thanks so much for opening this and welcome to Rust!

@imsnif beat me to responding, but here are a couple of additional comments I had :)

Firstly, I'm not certain what @imsnif had in mind, but I think I was imagining the elapsed time as only showing when the -t flag is used (at the moment it is always present). Personally, I'm not sure if I see too much of a benefit in having it displayed in rate mode, but that's certainly open for discussion.
It's purely stylistic, but I'd love to get some others' opinions on the colour and message text. "Total Elapsed Time" seems a little long, but I could be swayed. I'm also not certain about blue. It looks quite nice in your screenshots, but is a bit different in my terminal (see below).
At the moment, the elapsed time covers the total when the terminal is made small enough (see below). It's an edge case for sure, but the rest of the application is still usable when squished like that, so it might be good to handle that somehow.
Finally, while you are correctly handling when the application is paused, it does seem like a lot more code than just using instant.elapsed(). I'm wondering what both you and @imsnif would think about decoupling the pause feature of the UI and the call to ui.update_state(...). I'm thinking that, if we did that, the packet collection and timer could continue in the background while the UI is paused. That would help avoid missing packets and (I think) make this time tracking code much simpler.

Eosis · 2020-04-26T19:49:21Z

Hi @imsnif,

So I have spent a little time looking into the tests already and I think I have an understanding of what is required to fix them by manually looking at the snapshot files. Thanks for the tip about insta review, I will do that going forward instead... It is much less painful 😅

I am finding that the test failures are obivous new screen screen writes. There is a complication that the inherent differences in test run time means the exact writes may differ (sometimes they will show "00:00:01", and sometimes they will show "00:00:02").

I think we will either have to either:
a) Mock a timer to generate a time to not add randomness to the tests
b) Implement more general matching for this part of the UI to the tests

I think I prefer the latter of these, but would welcome any feedback. 😁

imsnif · 2020-04-26T20:00:20Z

Hey @Eosis - because of my less than ideal availability at the moment, I think I will leave you in the capable hands of @TheLostLambda. I hope this is okay with both of you? Do feel free to ping me for anything.

Eosis · 2020-04-26T20:09:48Z

Hi @TheLostLambda!

Thanks for the feedback and for the kind welcome. 😁

I agree I don't think it's very useful in the normal rate tests. I remember thinking that this area of the screen wasn't being used so it may be a nice addition. It is very easily changed whatever is decided. 👍
The text styling was a just a punt at getting something working 😄, Perhaps the safest option would be to keep the colour and style the same as for the Total Bandwidth Count in the top left?
Ah, yes, that's a problem. We will probably get away with just changing the rendering order. As an aside, is there a minimum width before the application terminates?
That's a tougher question that will need to be discussed. 😄

I agree that the suggested change would make the code for tracking the time much simpler, but as a user, I'd prefer the packet cap and parsing to stop at this point. This is coming from a resource perspective, where I am expecting to free up some processor time when I hit the pause.

That's just my thoughts, though, it would be good to hear other opinions on that. 😁

TheLostLambda · 2020-04-26T22:02:22Z

Hi @Eosis!

Don't feel super strongly either way, but I might prefer if it were just on the -t screen. I think that it would keep things a little bit cleaner, but that's just personal preference.
I think that might be worth a shot 😄 Depends on what you think looks best! Additionally, do you think it would be too confusing if it were just the time counter without "Total Time Elapsed: "? It would be a bit more compact that way, but I also don't want to confuse people.
Changing render order was the first thing that came to my mind as well. We could also do what all the tables do and drop columns (so the time would just not be printed if it doesn't fit). I don't think there is any size at which it terminates, but there probably isn't much point in designing it to work less than 40 columns or so, as that's about how wide the "Total Up / Down: ..." ends up being.
Hmm, I definitely see where you are coming from with that. I think I was coming at it from more of a "what's on screen right now" angle – the pause being a tool to see the current snapshot without interrupting the recording. But I agree it might be nice to actually free up those resources when it's paused. Now, with that being said, I've done some testing just now and I can't really pick out any CPU difference between paused or not. I think I'm bottle-necked by tui rendering to the terminal (which continues even when paused). I'd imagine it makes some difference when there is higher traffic though, or perhaps on different platforms (I think packet capture is a quite efficient kernel built-in on Linux).

I'd love to hear what you think about all of that! I'll also take a look into working around that snapshot issue with the tests, but I'm not certain what the best approach is at the moment.

Hope you are well!

- Only show the capture time on use of the utilization flag - Make colour consistent in header - Prioritize rendering of the Bandwidth over the duration - Reduce length of time description

Eosis · 2020-05-08T11:08:37Z

Hey @TheLostLambda,

Hope you're staying safe over there in Sheffield!

It took me a little while to get back to this. I've been busy with work and living the lockdown life.

We are now onlying show duration with -t flag usage
Total Time Elapsed HH:MM:SS > Duration HH:MM:SS. Also happy with HH:MM:SS if you'd prefer. All of the header is now a consistent colour. 😃
I've changed the rendering order for the moment to prioritize the bandwidth and will look into dropping the column in the header on screen-squish.
I think the general "Pause" behaviour is a larger topic than this issue. What do you think to closing this as-is and then opening a discussion about the way that "Pause" should work?

Have you had any more thoughts about working around the snapshots issue?

TheLostLambda · 2020-05-08T12:26:39Z

Hi @Eosis!

Thanks so much for the update, I didn't realize we were so nearby! I hope things are going well in Manchester as well!

To address your points:

I love it! Nice and simple code for it too!
I think Duration is perfect for HH:MM:SS, though I've realised it might be nice to display days as well. htop does something like: 27 days, HH:MM:SS. The days bit isn't fixed width and shows things like 1 day (there isn't an "s" there, so that's a bit of extra complexity) and 101 days is valid as well. If it's less than a day, it displays as HH:MM:SS only. If we went with this, I think it might be better to leave out Duration. While that's certainly more complex than the current approach, I think having days displayed would be very valuable. I am obviously open to other ways of displaying days though.
Stellar!
I'd tend to agree. I think we can go forward with this and address that separately.

I had a play with the snapshots just now, and it's a little hacky, but I think it's the minimum-code solution, so it's not much to change if we decide on something fancier later:

It just does a search and replace to change all of the 1 second tests to look like 2 seconds – you can then use cargo insta review to approve those and they should be the same every time.

Additionally, I think you may need to run cargo fmt to appease Travis (it uses cargo fmt -- --check to ensure code is consistently formatted).

Thanks so much for all of your great work!

imsnif · 2020-05-09T07:11:25Z

Hey, I hope you two don't mind me chiming in on the snapshot issue. We've had to deal with this in the raw_mode tests. I suspect in this case it will be a little more difficult, but maybe this approach can help you out? https://github.com/imsnif/bandwhich/blob/master/src/tests/cases/raw_mode.rs#L37-L43

Sorry for interrupting! Let me know if you need anything. :)

We now have the bandwidth data, paused status and duration in separate columns.

1, 2, or 3 sections will be used based on differing widths.

- Needed to set max number of columns to 3.

Eosis · 2020-05-09T16:35:44Z

Hey @TheLostLambda,

Yeah, not far at all! :)

👍
I've added the days and removed the duration. 😄
I've split the header into 3 sections (bandwidth, paused and duration) that are dropped somewhat dynamically, prioritizing the bandwidth print. This has had the effect of centering the "PAUSED" as well.
👍

As for the tests, I haven't looked at this again yet. I like your hacky solution as a stop gap. I'll look at the reference from @imsnif and assess the complexity in improving on the hacky way. 😁

TheLostLambda · 2020-05-09T17:35:04Z

Hi @Eosis!

I'm really liking how this is turning out! I've given it a go and come up with a couple of thoughts:

I'm certainly not opposed to using columns in the header, I think that's probably the proper approach, but we may need to do some more calculations for the width based on the actual string lengths. There is some strange behaviour where the bandwidth is cut off and the duration is dropped far sooner than it likely should be (see GIF). I think the easiest way out might just be sticking with the approach from before, but recalculating the layout depending on the length of the bandwidth string might also work.
Hmm, personally I'm not a massive fan of the centred pause, I quite liked the [PAUSED] next to the bandwidth, but how do you feel about it?
The days thing is looking really good! I think it might be nice to only display that when days is greater than zero. Something like:

# After a couple hours
03:14:15
# After a day and a bit
1 day, 12:42:13
# More than a day
4 days, 15:35:12

I'm open to thoughts on this though :)

Here is the GIF of odd column behaviour:

Thanks so much for all of the great work! As for tests, I think that either approach would work, though I may lean slightly towards the simpler .replace() method as I don't think Regex works as well on these TUI "frames" as they are differential and there isn't much of a pattern to match.

Hope you are doing well!

Now selectively printing out the duration of the capture based on calculating the exact length of the strings and the width of the terminal window. This has simplified this code substantially as well, no longer having to split the header into parts (currently we are printing to the same rect, but left and right aligned for the respective parts of the header).

Simply replacing the "1 \n" write events with "2 \n" when we check the snapshots to make them consistent. There may still be issues with timing in the tests, but can serve as a stop-gap.

Eosis · 2020-05-16T17:44:24Z

Hi @TheLostLambda ,

I think I've addressed your feedback and finally got the CI pipeline passing. :)

Have a look and let me know what you think.

I simplifed the header back to as it was, as the columns didn't seem to add a great deal with this simple case. This has had the effect of fixing the weird issue with the resizing.
I put it back to as it was as I don't have a strong preference. 👍
This is now implemented. 😁

TheLostLambda · 2020-05-16T20:58:46Z

src/main.rs

+                                ui.draw(
+                                    paused.load(Ordering::SeqCst),
+                                    dns_shown,
+                                    std::time::Duration::new(131, 0),


I think we should probably replace this with the actual duration :)

Yeah, figuring out why when resizing the window the timer changes to 02:11 were a confusing 2 minutes. ;P

Opps... Sorry! 😅

TheLostLambda · 2020-05-16T21:05:10Z

Hi @Eosis!

I've given it a good poke around, testing the scaling and the behaviour after some simulated days have passed and it's looking absolutely fantastic!

I left one comment where we need to replace a placeholder value, but aside from that, I think it's looking perfect! If it's alright, I might spend some time tomorrow looking over the code before merging, but I'm super happy with this :)

I'll also ping @imsnif to see what he thinks as well.

Thank you so much for all of your hard work and I'm looking forward to merging this as soon as we get that placeholder filled in!

imsnif

This looks great to me. Just left a minor comment. Good work! @TheLostLambda - feel free to merge if it looks good to you too.

imsnif · 2020-05-17T13:25:22Z

src/display/components/total_bandwidth.rs

    pub paused: bool,
 }

-impl<'a> TotalBandwidth<'a> {
+impl<'a> HeaderDetails<'a> {


Hey, good call on the name change! One little thing though, could we also change the file name? Otherwise I'm afraid it might get a little confusing.

I agree and have changed the name of the file. 👍

imsnif · 2020-05-17T13:39:31Z

src/main.rs

+                                ui.draw(
+                                    paused.load(Ordering::SeqCst),
+                                    dns_shown,
+                                    std::time::Duration::new(131, 0),


Yeah, figuring out why when resizing the window the timer changes to 02:11 were a confusing 2 minutes. ;P

imsnif · 2020-05-17T13:40:51Z

src/display/components/total_bandwidth.rs


+        if print_elapsed_time {
+            self.render_elapsed_time(frame, rect, elapsed_time.as_ref().unwrap(), color);


Hah! Nice one :)

imsnif · 2020-05-18T08:05:39Z

(also, feel free to reach out if there are issues or uncertainties with the merge conflicts)

- Split the logic for determining the elapsed time into a function as it was called in different placed in main.rs.

Conflicts: src/display/components/layout.rs src/display/ui.rs src/main.rs

Eosis · 2020-05-20T19:12:49Z

Hey both,

I have fixed the conflicts from the merge and am hopeful the CI will pass. 🤞

Let me know if you have any more comments. 😁

TheLostLambda · 2020-05-20T19:15:06Z

Awesome! If CI and everything checks out, I'll give the code a proper look tomorrow then we can get this merged! Thanks for all of your work on this!

TheLostLambda · 2020-05-21T10:35:47Z

@imsnif Hmm, could we try restarting this Travis build? It looks like some behaviour has changed on the beta Rust build for MacOS? I can't restart the job myself unfortunately. It would be good to rule out just some one-time failure.

imsnif · 2020-05-21T11:34:04Z

@TheLostLambda - just restarted. Odd that you couldn't restart it yourself... are you logged into Travis with your Github account? If so, I'll try to see what's up with it.

TheLostLambda · 2020-05-21T11:40:39Z

@imsnif Whoops, definitely wasn't logged in. That's my bad, I haven't used Travis before! I can see the restart option now :)

imsnif · 2020-05-21T11:44:26Z

Cool, no worries :)

TheLostLambda · 2020-05-21T12:04:59Z

@Eosis Things are looking stellar! I went through the code and found a couple of little tweaks that could be made, but I'm happy merging with or without them. You can take a look here integrate any tweaks you like and just let me know when you are looking to merge!

imsnif · 2020-05-21T13:58:38Z

Also @Eosis - once this is merged I'm planning on issuing a new release and writing (among other things) about this feature on twitter. I'd be happy to link to your twitter if you send me a link (or I can link to your github too - as you prefer). Thanks again for your hard work!

@TheLostLambda

Thanks @TheLostLambda

Eosis · 2020-05-23T11:12:23Z

Thanks for the feedback @TheLostLambda. I implemented the bits in main but I think I prefer the originals in header_details.rs. I think we are good to merge now. 😁👍

Hi @imsnif, Linking GH and twitter would be fun. My twitter handle is @RupRutledge. 😁

TheLostLambda · 2020-05-23T11:14:14Z

Looks excellent! Thanks for all of the hard work! Merging now! 😄

Rupert Rutledge added 6 commits April 26, 2020 14:12

Running cargo fmt

5cb0c76

Split Header rendering into different sections

8a2469f

- Rename Bandwidth to HeaderDetails to indicate the added information it now contains. - Split the rendering function for the header into its constituent parts

Change from Mutex to RwLock around elapsed time variables

265390b

A RwLock should be more performant.

Running cargo fmt

4844693

Add space to elapsed time to mirror other print

1e7e0e4

The bandwidth print has a leading space, so this should have a trailing space.

Address PR feedback.

598239a

- Only show the capture time on use of the utilization flag - Make colour consistent in header - Prioritize rendering of the Bandwidth over the duration - Reduce length of time description

Rupert Rutledge added 5 commits May 9, 2020 15:48

Split the header row into 3 columns

a177280

We now have the bandwidth data, paused status and duration in separate columns.

Split the header into different parts based on size of terminal

da17936

1, 2, or 3 sections will be used based on differing widths.

Simplify logic in the header_parts function

0122bf3

Fix silly issue with altering logic

53b80c7

- Needed to set max number of columns to 3.

Add days to the duration print

e17f8e6

Rupert Rutledge added 5 commits May 16, 2020 17:23

Fix issue of duration being shown on non -t invocations

2493496

Altering the snapshots for tests that are affected by new duration code

07c734a

Simply replacing the "1 \n" write events with "2 \n" when we check the snapshots to make them consistent. There may still be issues with timing in the tests, but can serve as a stop-gap.

Merge remote-tracking branch 'origin/master' into issue-163-elapsed-time

18de3a1

Deal with clippy linting issues

edea995

TheLostLambda reviewed May 16, 2020

View reviewed changes

TheLostLambda requested a review from imsnif May 16, 2020 21:07

imsnif reviewed May 17, 2020

View reviewed changes

Rupert Rutledge added 2 commits May 18, 2020 21:40

Rename the total_bandwidth file for clarity

098af1f

Use correct elapsed time when resizing the terminal

d897e14

- Split the logic for determining the elapsed time into a function as it was called in different placed in main.rs.

Eosis force-pushed the issue-163-elapsed-time branch from e1615c9 to d897e14 Compare May 19, 2020 20:31

Rupert Rutledge added 2 commits May 20, 2020 20:00

Merge remote-tracking branch 'origin/master' into issue-163-elapsed-time

49aef18

Conflicts: src/display/components/layout.rs src/display/ui.rs src/main.rs

Update changelog

83de3de

Implement style feedback

b6f7f42

Thanks @TheLostLambda

TheLostLambda merged commit e9b8a18 into imsnif:master May 23, 2020

Eosis deleted the issue-163-elapsed-time branch May 24, 2020 14:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 163 elapsed time #167

Issue 163 elapsed time #167

Eosis commented Apr 26, 2020

imsnif commented Apr 26, 2020

TheLostLambda commented Apr 26, 2020

Eosis commented Apr 26, 2020

imsnif commented Apr 26, 2020

Eosis commented Apr 26, 2020

TheLostLambda commented Apr 26, 2020

Eosis commented May 8, 2020

TheLostLambda commented May 8, 2020

imsnif commented May 9, 2020

Eosis commented May 9, 2020

TheLostLambda commented May 9, 2020

Eosis commented May 16, 2020

TheLostLambda May 16, 2020

imsnif May 17, 2020

Eosis May 20, 2020

TheLostLambda commented May 16, 2020

imsnif left a comment

imsnif May 17, 2020

Eosis May 20, 2020

imsnif May 17, 2020

imsnif May 17, 2020

imsnif commented May 18, 2020

Eosis commented May 20, 2020

TheLostLambda commented May 20, 2020

TheLostLambda commented May 21, 2020

imsnif commented May 21, 2020

TheLostLambda commented May 21, 2020

imsnif commented May 21, 2020

TheLostLambda commented May 21, 2020

imsnif commented May 21, 2020

Eosis commented May 23, 2020

TheLostLambda commented May 23, 2020


		if print_elapsed_time {
		self.render_elapsed_time(frame, rect, elapsed_time.as_ref().unwrap(), color);

Issue 163 elapsed time #167

Issue 163 elapsed time #167

Conversation

Eosis commented Apr 26, 2020

imsnif commented Apr 26, 2020

TheLostLambda commented Apr 26, 2020

Eosis commented Apr 26, 2020

imsnif commented Apr 26, 2020

Eosis commented Apr 26, 2020

TheLostLambda commented Apr 26, 2020

Eosis commented May 8, 2020

TheLostLambda commented May 8, 2020

imsnif commented May 9, 2020

Eosis commented May 9, 2020

TheLostLambda commented May 9, 2020

Eosis commented May 16, 2020

TheLostLambda May 16, 2020

Choose a reason for hiding this comment

imsnif May 17, 2020

Choose a reason for hiding this comment

Eosis May 20, 2020

Choose a reason for hiding this comment

TheLostLambda commented May 16, 2020

imsnif left a comment

Choose a reason for hiding this comment

imsnif May 17, 2020

Choose a reason for hiding this comment

Eosis May 20, 2020

Choose a reason for hiding this comment

imsnif May 17, 2020

Choose a reason for hiding this comment

imsnif May 17, 2020

Choose a reason for hiding this comment

imsnif commented May 18, 2020

Eosis commented May 20, 2020

TheLostLambda commented May 20, 2020

TheLostLambda commented May 21, 2020

imsnif commented May 21, 2020

TheLostLambda commented May 21, 2020

imsnif commented May 21, 2020

TheLostLambda commented May 21, 2020

imsnif commented May 21, 2020

Eosis commented May 23, 2020

TheLostLambda commented May 23, 2020