Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add network bandwidth support for macOS #249

Merged
merged 3 commits into from
Jan 30, 2024

Conversation

emmagamma
Copy link
Contributor

@emmagamma emmagamma commented Jan 30, 2024

So I went to enable network-bandwidth (on a mac) and noticed this wasn't supported yet, but it was only after getting some custom changes working locally that I even thought to check if there was already a PR for it haha.

I see #113 exists already, but it looks like the person who made that PR just got busy with work and hasn't had time to address all the feedback and resolve the conflicts, and progress seems to have stalled on that several years ago - so I really hope I'm not stepping on any toes by making a separate PR for the same feature, but I'd really love to get this merged into the repo so that I don't need to run a modified version of this repo and manually keep it in sync with my local changes when I update to newer versions.

please let me know if there's anything I should change, happy to ^.^
(I figure you'll probably want me to squash the commits, but I'll leave them as-is for now, for convenience)

and some notes to hopefully make reviewing this a bit easier:

  • Uses route instead of ip to get the interface name on Darwin Architectures, which is installed by default on macOS.
  • Uses basically the same approach as in Add support for displaying network bandwidth usage in macOS #113 with netstat to get the tx and rx bytes, except instead of calling netstat twice, we can just do it once and return both values at the same time.
  • Updated the interface_bytes fn to no longer require a second argument ($2 which would always either be "tx" or "rx"), so instead now it just gets the interface name from $1 (the first argument) and returns BOTH the tx and rx bytes for that interface, in the format: "TX_Bytes RX_Bytes" where the upload (tx) bytes are listed first, as is the convention elsewhere, and then a whitespace character followed by the download (rx) bytes. This lets us reduce the number of calls to interface_bytes per interval from 4 to 2 while also making the code in get_bandwidth a bit simpler and easier to read.
  • And the last commit is not really required to support network-bandwidth on macOS (so I can revert if you don't think this is a good change), but I also switched out the line using bc to use awk for floating point math, the "%.2f" limits the output of printf to two decimal places. I believe it should be reasonable to expect awk to be installed already on most OS's and the repo's existing patterns seem to agree but please lmk if that's not a sound assumption.

@emmagamma
Copy link
Contributor Author

And here's a little demo of it working, side-by-side with activity monitor:

macos-network-bandwidth-demo.mov

you'll notice activity monitor updates more frequently than once per second, so it's showing a higher-resolution picture of your traffic and may occasionally display higher and lower spikes for tx/rx compared to the network-bandwidth in tmux. but overall they both give a very accurate indication of your traffic.

@ethancedwards8
Copy link
Member

This is wonderful, thank you so much! I definitely agree that it is more reasonable to expect awk to be installed. I have noticed that bc is not installed on many systems. No need to squash commits, everything is very clean and well written.

@ethancedwards8 ethancedwards8 merged commit a56a5f6 into dracula:master Jan 30, 2024
1 check passed
@emmagamma
Copy link
Contributor Author

sweet! wow that was incredibly fast haha I totally expected more back and forth, thanks for the quick merge <3

@ethancedwards8
Copy link
Member

sweet! wow that was incredibly fast haha I totally expected more back and forth, thanks for the quick merge <3

You got everything right the first time! That's pretty rare around these parts ;). I can tell that you poked around the codebase to see what the code standards are, I'm impressed. Thanks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants