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

(Re)add etime and cmd into process stats #175

Merged
merged 5 commits into from
Sep 12, 2024
Merged

Conversation

asmacdo
Copy link
Member

@asmacdo asmacdo commented Sep 4, 2024

Fixes #174

@asmacdo asmacdo added semver-minor Increment the minor version when merged release Create a release when this pr is merged labels Sep 4, 2024
@asmacdo asmacdo requested review from yarikoptic and jwodder and removed request for jwodder September 4, 2024 21:40
Copy link

codecov bot commented Sep 4, 2024

Codecov Report

Attention: Patch coverage is 72.72727% with 3 lines in your changes missing coverage. Please review.

Project coverage is 94.92%. Comparing base (7057bf2) to head (9cb2218).
Report is 14 commits behind head on main.

Files with missing lines Patch % Lines
src/con_duct/__main__.py 72.72% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #175      +/-   ##
==========================================
- Coverage   95.42%   94.92%   -0.50%     
==========================================
  Files           2        2              
  Lines         459      473      +14     
  Branches       70       73       +3     
==========================================
+ Hits          438      449      +11     
- Misses         10       12       +2     
- Partials       11       12       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@asmacdo asmacdo marked this pull request as draft September 4, 2024 21:50
@asmacdo
Copy link
Member Author

asmacdo commented Sep 4, 2024

Tests are failing on AssertionError assert self.cmd == other.cmd

This shouldn't happen, could indicate a bigger issue-- process stat aggregation can only occur for the same process/cmd. I'll investigate whats going on with that.

@yarikoptic
Copy link
Member

And don't we have some integration test with known children processes ? (Some sleeps etc?)

@yarikoptic
Copy link
Member

and note that it doesn't fail consistently, so it is something flaky or incorrect assumption somewhere!

@asmacdo
Copy link
Member Author

asmacdo commented Sep 11, 2024

2 failing tests were:

  • Test / test (ubuntu-latest, 3.12, py) (pull_request)
  • Test / test (ubuntu-latest, pypy-3.9, py) (pull_request)

Rerunning to see if its flake or version dependent.

If we catch a sample between exit and parent process termination,
the cmd is changed, and the stats aren't really valuable anymore.

Since this PR added a sanity check to ensure that we do not consolidate
stats for processes with different commands, this occassionally caused an error.
This abbreviation happens at the kernel level, and ps surrounds it in
brackets to indicate that it has changed.
@asmacdo asmacdo marked this pull request as ready for review September 11, 2024 20:45
@asmacdo
Copy link
Member Author

asmacdo commented Sep 11, 2024

All checks passed. Rerunning CI for a sanity check since this has been flakey.

Co-authored-by: Yaroslav Halchenko <[email protected]>
@asmacdo asmacdo merged commit ba04e03 into con:main Sep 12, 2024
13 of 15 checks passed
@asmacdo asmacdo deleted the etime-cmd branch October 4, 2024 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release Create a release when this pr is merged semver-minor Increment the minor version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

collect command for each pid
2 participants