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

[checks] [process] add pagefault stats #2477

Merged
merged 3 commits into from
May 13, 2016
Merged

Conversation

gmmeyer
Copy link
Contributor

@gmmeyer gmmeyer commented May 6, 2016

Mostly from PR #2363.

I fixed the failing test and rebased against master.

@gmmeyer gmmeyer force-pushed the greg/indeedeng/pagefaultstats branch 2 times, most recently from 6636b18 to 65cc916 Compare May 6, 2016 17:32
@gmmeyer gmmeyer added this to the 5.8.0 milestone May 6, 2016
@gmmeyer
Copy link
Contributor Author

gmmeyer commented May 6, 2016

I cancelled the failed check, it's just two lines of comments and I didn't think it was necessary to run the whole test suite again for two comments. The checks got run in the previous commit and passed.

# http://man7.org/linux/man-pages/man5/proc.5.html
try:
data = file_to_string('/proc/%s/stat' % pid)
except:
Copy link
Member

Choose a reason for hiding this comment

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

let's avoid the bare exception here, and do except Exception: instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay

@olivielpeau olivielpeau self-assigned this May 6, 2016
@olivielpeau
Copy link
Member

@gmmeyer Added some comments. Could you also squash the commits you authored into one?

@gmmeyer
Copy link
Contributor Author

gmmeyer commented May 6, 2016

Yes I can squash them

@gmmeyer gmmeyer force-pushed the greg/indeedeng/pagefaultstats branch from 8878de4 to 021ec81 Compare May 6, 2016 21:52
@gmmeyer
Copy link
Contributor Author

gmmeyer commented May 6, 2016

The failure was activemq. I'm pretty sure it's unrelated. I can run the tests again if you want.

@gmmeyer gmmeyer force-pushed the greg/indeedeng/pagefaultstats branch from 021ec81 to 296cbdd Compare May 9, 2016 15:38
@@ -34,6 +34,10 @@
'w_bytes': 'iowrite_bytes', # FIXME: namespace me correctly (6.x) io.w_bytes
'ctx_swtch_vol': 'voluntary_ctx_switches', # FIXME: namespace me correctly (6.x), ctx_swt.voluntary
'ctx_swtch_invol': 'involuntary_ctx_switches', # FIXME: namespace me correctly (6.x), ctx_swt.involuntary
'minflt': 'mem.minflt',
Copy link
Member

@truthbk truthbk May 9, 2016

Choose a reason for hiding this comment

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

Wondering if we should keep these names (mem.minflt, mem.cminflt, etc...) as (presently) defined on the manpage - or if we should make them more human readable. Let's check with the team....

@olivielpeau olivielpeau assigned truthbk and unassigned olivielpeau May 9, 2016
@gmmeyer gmmeyer force-pushed the greg/indeedeng/pagefaultstats branch from 146200a to 54c07e2 Compare May 9, 2016 20:30
@gmmeyer
Copy link
Contributor Author

gmmeyer commented May 12, 2016

I've addressed all of these issues in the most recent commit. Let me know if there's anything else. 😃

@truthbk
Copy link
Member

truthbk commented May 13, 2016

👍 looks good!

@gmmeyer gmmeyer merged commit 08e2bfc into master May 13, 2016
@gmmeyer gmmeyer deleted the greg/indeedeng/pagefaultstats branch May 17, 2016 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants