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.

Sign up for GitHub

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

Jump to bottom

Included mountstats and nfsstats in logs #141

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Included mountstats and nfsstats in logs #141

wants to merge 2 commits into from

Conversation

ksub42
Copy link
Collaborator

@ksub42 ksub42 commented May 13, 2023

  1. Included mountstats & nfsstats in the logs that we collect.
  2. Cleaned up code and fixed two misc bugs (one in arg parsing, one in killing tcpdump).
  3. Fixed Python invocation to be independend of the actual location of the Python interpreter in trace-nfsbpf.

@ksub42
Included mountstats and nfsstats in logs
6a58902 
  1. Included mountstats & nfsstats in the logs that we collect.
  2. Cleaned up code and fixed two misc bugs (one in arg parsing,
     one in killing tcpdump).
  3. Fixed Python invocation to be independend of the actual location of
     the Python interpreter in trace-nfsbpf.
@ksub42 ksub42 requested a review from bharathsm-ms May 13, 2023 10:01
sprasad-microsoft
sprasad-microsoft reviewed May 16, 2023
View reviewed changes
NfsDiagnostics/nfsclientlogs.sh Outdated
fi

# trace-nfsbpf
ABSOLUTE_PATH="$(cd "$(dirname "trace-nfsbpf")" && pwd)/$(basename "trace-nfsbpf")"
progfile2="nohup $ABSOLUTE_PATH"
NFSBPF_ABSOLUTE_PATH="$(cd "$(dirname "trace-nfsbpf")" && pwd)/$(basename "trace-nfsbpf")"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not simply do:
$ realpath trace-nfsbpf

sprasad-microsoft
sprasad-microsoft approved these changes May 16, 2023
View reviewed changes
Copy link
Collaborator

@sprasad-microsoft sprasad-microsoft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall changes look good. Just had a minor comment.
Please do some testing and merge the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sprasad-microsoft sprasad-microsoft sprasad-microsoft approved these changes

@bharathsm-ms bharathsm-ms Awaiting requested review from bharathsm-ms

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ksub42 @sprasad-microsoft