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

Move the memray attach callback into a module #459

Closed
wants to merge 5 commits into from

Conversation

godlygeek
Copy link
Contributor

Previously we were maintaining this as a string literal inside the
client-side attach code, but it's growing more complex, and we're
reaching the point where it would be advantageous to have it in a normal
module that linters and type checkers can analyze.

This commit is based on the code in #455, and will need to be rebased once we land that, but it's ready for review other than that. Marking it draft until #455 is landed.

ivonastojanovic and others added 5 commits September 8, 2023 18:55
Added --aggregate option which allows user to request aggregated mode for in-memory aggregation.

Signed-off-by: Ivona Stojanovic <[email protected]>
Added a unit and an integration test for the --aggregate option. The unit test tests the case when --aggregate is given but --output is not. The --aggregate option is tested in the integration test.

Signed-off-by: Ivona Stojanovic <[email protected]>
Previously we were maintaining this as a string literal inside the
client-side `attach` code, but it's growing more complex, and we're
reaching the point where it would be advantageous to have it in a normal
module that linters and type checkers can analyze.

Signed-off-by: Matt Wozniski <[email protected]>
@codecov-commenter
Copy link

Codecov Report

Patch coverage: 80.28% and project coverage change: -0.11% ⚠️

Comparison is base (bb9c0db) 91.88% compared to head (d47422d) 91.77%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #459      +/-   ##
==========================================
- Coverage   91.88%   91.77%   -0.11%     
==========================================
  Files          90       92       +2     
  Lines       10766    10822      +56     
  Branches     1473     1486      +13     
==========================================
+ Hits         9892     9932      +40     
- Misses        872      888      +16     
  Partials        2        2              
Flag Coverage Δ
cpp 84.99% <ø> (-0.25%) ⬇️
python_and_cython 95.23% <80.28%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/memray/_attach_callback.py 22.22% <22.22%> (ø)
src/memray/commands/attach.py 59.41% <100.00%> (+5.47%) ⬆️
tests/integration/test_attach.py 91.22% <100.00%> (+5.11%) ⬆️
tests/unit/test_attach.py 100.00% <100.00%> (ø)

... and 5 files with indirect coverage changes

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

@godlygeek
Copy link
Contributor Author

Actually... I've realized this will break using a new version memray to attach to a Python process where an older version of memray is installed. Now, that's not really something we've ever officially supported, but it is something that has always mostly worked - the live TUI wouldn't work if the client and server use a different version of our record format, but I believe attaching a tracker that outputs to a capture file has always worked, and that's something that our users might be relying on without us realizing it. That's a strong argument for sticking with having the client inject the code to be executed, rather than having the server import it from its installed copy of memray.

@godlygeek
Copy link
Contributor Author

Let's not do this. It doesn't work as nicely as I thought it would.

@godlygeek godlygeek closed this Nov 7, 2023
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.

4 participants