Added new commands to memray attach

[ivonastojanovic]

Letting memray attach supports deactivating an attached tracker manually, or after a specified heap size is reached, or after a specified time limit has elapsed.

Issue number of the reported bug or feature request: #

Describe your changes
A clear and concise description of the changes you have made.

Testing performed
Describe the testing you have performed to ensure that the bug has been addressed, or that the new feature works as planned.

Additional context
Add any other context about your contribution here.

[godlygeek]

Let's avoid mixing dashes and underscores. Also... Hm, let's call this "heap limit" instead.

Suggested change

	"--heap_size", type=int, help="Heap size to track until (in bytes)"
	"--heap-limit", type=int, help="Heap size to track until (in bytes)"

[godlygeek]

We don't do any logging here, so this call and the import logging aren't needed

[godlygeek]

This isn't going to work the way you want. You're trying to share these globals across multiple calls to attach, but each attach call runs its own script, with its own globals namespace. That's why we do the memray._last_tracker = ... thing below: we're sticking the tracker, which we need to be shared across calls to memray attach, in a global variable of the memray module, so that future calls from different namespaces can share it.

Honestly, this is probably a reason to split this out into a proper module. This is getting too complicated to manage as an inline script like this... I'm going to take a swing at refactoring this and giving you something easier to work with for basing your changes on top of...

[godlygeek]

OK, the last commit on #459 gives you something saner to base this on. Rather than just a giant string literal, that PR will give you a new module named memray._attach_callback that we can put these changes in, to help make managing the global state easier.

The memray._attach_callback.callback function will be the entry point. It'll take both the arguments that are going to be passed to the Tracker constructor as well as the arguments determining which attach mode to use, and it will call other functions in the module to handle the tracker creation and destruction, as well as the threads to disable tracking when different thresholds are reached.

[godlygeek]

Let's instead go with a boolean option named --stop-tracking, so that the user runs:

memray attach --stop-tracking $pid

[godlygeek]

We should have some extra checking that these different modes can't be missed. As it is, the user is allowed to run memray attach --mode=deactivate --duration=10 --output=foo.bin which doesn't make any sense. We should prevent --deactivate-tracking from being used with any other option except --method, and we should prevent --heap-limit and --duration from being used together.

[codecov-commenter]

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (baf7f41) 91.96% compared to head (56d5b3a) 92.15%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #458      +/-   ##
==========================================
+ Coverage   91.96%   92.15%   +0.19%     
==========================================
  Files          91       91              
  Lines       10790    10849      +59     
  Branches     1485     1498      +13     
==========================================
+ Hits         9923     9998      +75     
+ Misses        865      849      -16     
  Partials        2        2              

Flag	Coverage Δ
cpp	85.68% <ø> (+0.43%)	⬆️
python_and_cython	95.44% <90.10%> (+0.03%)	⬆️

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

Files	Coverage Δ
tests/unit/test_attach.py	100.00% <100.00%> (ø)
tests/integration/test_attach.py	96.47% <94.28%> (+5.24%)	⬆️
src/memray/commands/attach.py	64.32% <87.27%> (+5.15%)	⬆️

... and 3 files with indirect coverage changes

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

[godlygeek]

I've pushed quite a few fixup commits here to make various improvements. Most of them are pretty small things. The biggest one is refactoring the thread for the heap size check so that it can be cancelled, like the duration check one can. I also moved the global state to be attributes of the memray module, as we discussed above.

@ivonastojanovic Please review my changes and let me know if you spot any mistakes, bugs, issues, or shortcomings. Each of the fixup commits has a short explanation of why it was made, but I typed those up quickly, so also let me know if you don't understand the purpose of one of the changes.

[godlygeek]

At this point this LGTM, but since I made some fairly extensive tweaks here, I'd appreciate it if @pablogsal could give this a read before we land this and see if any issues jump out.

[pablogsal]

Left a nit, otherwise LGTM

[pablogsal]

@godlygeek I assume testing these options may be too cumbersome but maybe we should do some smoke testing at least....

[godlygeek]

In the first version of this we weren't logging anything when detaching, and so it was essentially impossible to prove that the new code was reached - but now that we're logging, I suppose we could do some simple smoke tests that just check the stdio of the tracked process looking for "heap size has reached" and "seconds have elapsed". Think that's good enough?

[pablogsal]

Yeah, that will certainly work

[pablogsal]

Check de94a39

[godlygeek]

Well, that doesn't pass CI 😅

The more I think about this, the more I become convinced that this should be a separate subcommand instead. Rather than having a memray attach --stop-tracking that's mutually exclusive with almost every other memray attach argument, we should just have a separate memray detach command. At first I thought that wouldn't make sense because memray detach would still need to attach to the process in order to uninstall the old tracker, but I think that's just me letting my knowledge of the underlying architecture and communication channels cloud my judgment. I think that a typical user would look at memray attach as attaching Memray to the process, as opposed to gdb or lldb, and so users would find it perfectly intuitive for there to be a memray detach command that removes Memray from the process.

Let me know if you agree - if you do, I'll do the legwork of splitting this into a separate memray detach subcommand.

[godlygeek]

OK, I've updated this PR to make memray detach a separate command, and to fix the small issue with your fixup that caused the CI to fail. Take a look when you can, @pablogsal

[godlygeek]

Oh, and https://godlygeek.github.io/memray/attach.html#cli-reference shows how the docs changes render.

[godlygeek]

After some offline discussions, @pablogsal and I have decided to drop --heap-limit from this PR. The name is misleading, since the implementation is looking at maximum RSS rather than at the size of the heap, and it refuses to work reliably on i686 for reasons we haven't managed to track down (operations that we expect to be raising ru_maxrss appear not to, though only on i686 and not on x86-64 or arm64).

Instead, our plan is to add a way to set these limits when Tracker is created and installed, and have it enforce the limits itself. That change will come in a future release, though. For now, we're going to remove the option from this PR to unblock the way for including the rest of this work in a release.