search

facebookincubator / below

A time traveling resource monitor for modern Linux systems
Apache License 2.0
1.04k stars 60 forks source link

Add support for recording ethtool stats #8204

Closed mmynk closed 11 months ago

mmynk commented 1 year ago

The PR is broken into modular commits, so it might be easier to review the PR commit-by-commit.

Commit: Add crate for reading NIC stats using ethtool

Commit: Add model to represent ethtool stats

Commit: Add render configs for ethtool fields

Commit: Add ability for printing raw_stats in OpenMetrics format

dschatzberg commented 1 year ago

@mmynk Thanks for submitting this PR. I probably won't have time to look until the end of the week, but just acknowledging that I've seen the submission - sorry for the slow response.

mmynk commented 1 year ago

@dschatzberg no worries, take your time. thanks for the ack!

mmynk commented 1 year ago

A lot of this commit should be squashed into the one before, the model changes look reasonable

should I squash commits in the PR or is it okay to do so while merging?

dschatzberg commented 1 year ago

Sorry, Github doesn't make it clear that I was referring to the 2nd commit. Please squash in the PR - to be clear, I'm fine with multiple commits per PR, in fact, I prefer it. But, many of the changes in the 2nd commit are logically more a part of the first commit which makes it harder to review.

Personally, I like to rewrite my commits after writing the full PR - e.g. via https://git-scm.com/book/en/v2/Git-Tools-Interactive-Staging

mmynk commented 1 year ago

@dschatzberg Thanks for the detailed review, have addressed most of the comments. Could you please take a look again? Build is failing because of recent dateutil changes, fixed in #8206

mmynk commented 1 year ago

@dschatzberg have removed all the re-defined structs and used structs from the binding, this led to a few unsafe calls, hope that's okay?[1]

[1] https://github.com/facebookincubator/below/pull/8204/commits/e316ca0e51660adeb57ac147f67eb80a652712da

mmynk commented 1 year ago

@dschatzberg could you please re-review the PR? thanks!

dschatzberg commented 1 year ago

@mmynk Sorry I've been behind on some other work. I'm out on vacation all next week so unlikely I'll get to look at this for the next week but I'll review once I'm back if not.

mmynk commented 1 year ago

hey @dschatzberg, just wanted to drop a gentle reminder. thanks!

dschatzberg commented 12 months ago

@mmynk Sorry for the delay, I'm working on the review and should get you feedback this week

mmynk commented 12 months ago

@dschatzberg thank you! Have addressed the comments. Please take a look whenever possible.

mmynk commented 11 months ago

@dschatzberg gentle reminder, thanks!

facebook-github-bot commented 11 months ago

@dschatzberg has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot commented 11 months ago

@dschatzberg merged this pull request in facebookincubator/below@7fc6f7f7e1bdee0716a49bf2ab4a44e1d3623dc7.

dschatzberg commented 11 months ago

@mmynk Just a heads up, I changed a couple things in the PR when integrating it internally:

1) There's a config option now to enable gathering ethtool stats - we need this for rollout safety so we can push code and dynamically turn things off/on separately. It defaults to disable but if we have no issues rolling it out I'd be happy to flip that.

2) I ended up committing the bindgen'd file itself because there's no pre-existing library that includes the ethtool header so it made it a bit challenging to generate in our repository where all dependencies must be vendored.

mmynk commented 11 months ago

@dschatzberg Do you see value in syncing those changes in upstream?

dschatzberg commented 11 months ago

@mmynk I already did sync those changes (e.g. https://github.com/facebookincubator/below/commit/7fc6f7f7e1bdee0716a49bf2ab4a44e1d3623dc7#diff-080793436ea7d5d338322a440a1430f13a281c6e832366a2f03ea641a44c797eR63) as part of commiting this PR.

Just so it's clear - the source of truth for Below is Meta's internal code repository and we have automation to sync changes from that to this repository on Github. Every PR you submit has to land first in our internal code repository before it makes it to Github. So the term "upstream" can be a bit confusing.