Print the thread name in the live TUI #562
Conversation
We're performing formatting in this method, rather than exposing the thread's name directly. Rename this method to make it clear that it's performing some formatting, and to unblock adding a new method exposing the thread name directly. Signed-off-by: Matt Wozniski <[email protected]>
Signed-off-by: Matt Wozniski <[email protected]>
To allow users to determine thread in a multithreaded application corresponds to the "Thread X of Y" number displayed in the live view, include the thread name that we collect in the TUI. Signed-off-by: Pablo Galindo <[email protected]> Signed-off-by: Matt Wozniski <[email protected]>
There was a problem hiding this comment.
As we discussed offline, I made a bunch of changes.
I renamed AllocationRecord.thread_name to pretty_thread_name to make it clear that it's doing some string formatting and not just returning data from the record, and then added a new thread_name attribute that is just the data from the record.
Given that AllocationRecord is intended to (eventually) be part of our public interface, it probably shouldn't have pretty_thread_name at all, and we should instead move that to a function in common.py that our reporters can use, but that's a problem for another day.
Anyway, on top of that change, I updated this to only show a thread name if we have one, so the changes to the Textual snapshot cases are reverted (none of those threads have names). I also updated one of the tests to exercise the case where threads do have names, and I tested it against a Bloomberg service where threads do have names to show that it does work as intended.
I'm approving this since you won't be able to approve your own PR, but please do check my work before you land this, @pablogsal.
|
Not really a fan of having |
Me neither, but I don't think If you'd like me to revert the changes to have both thread_name = record.thread_name
thread_name = thread_name[thread_name.find("(") : thread_name.rfind(")") + 1]which, yuck. Alternatively (and I think preferably), I could push forward with my cleanup by removing
It doesn't, but it does display the thread name when we have the thread name, which is more than never. It also makes it so that the only piece missing for solving #561 is for us to capture the Python thread name in the tracker as well.
Well, flipping this to draft until we decide, but it seems to me that this is already a reasonably sized self-contained PR that moves the ball forward and displays information that we already collect and which can be useful to people using the TUI. I think #561 can really be broken down into 2 related work items:
We need both pieces, and I don't think both need to be in the same PR (or even the same release). This is already useful today in applications that use |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #562 +/- ##
==========================================
+ Coverage 92.55% 92.99% +0.44%
==========================================
Files 91 93 +2
Lines 11304 11281 -23
Branches 1581 2060 +479
==========================================
+ Hits 10462 10491 +29
+ Misses 837 790 -47
+ Partials 5 0 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
godlygeek
force-pushed
the
thread_name
branch
5 times, most recently
from
634b780
to
dc73a0d
Compare
Previously we only captured the thread name as set by `prctl`, but not the thread name as returned by `threading.current_thread().name`. Begin capturing the name for the Python thread as well. We retain only the last name set for each thread, so assignments to `Thread.name` override earlier calls to `prctl(PR_SET_NAME)`, and vice versa. This implementation uses a custom descriptor to intercept assignments to `Thread._name` and `Thread._ident` in order to detect when a thread has a name or a thread id assigned to it. Because this is tricky and a bit fragile (poking at the internals of `Thread`), I've implemented that descriptor in a Python module. At least that way if it ever breaks, it should be a bit easier for someone to investigate. Signed-off-by: Matt Wozniski <[email protected]>
Introduce a new `memray.reporters.common` module for utilities needed by multiple reporters. Replace the `AllocationRecord.pretty_thread_name` property with a `format_thread_name` function in that new module. Signed-off-by: Matt Wozniski <[email protected]>
I've done this (last commit on this PR)
I've implemented item 1 (second to last commit on this PR), using the descriptor-based plan we discussed offline earlier today. It turned out to be a bit more involved than it seemed at first glance, because we needed to worry about names being set before a thread is started, or one thread's name being set from a different thread. It turned out not to be too tough to support both of those things, though. Before you ask: there's a C-style cast in this PR to convert a
I stopped wrestling with it after a while and just switched to a C-style cast, so that it reinterprets when we need reinterpreting and statically casts when we need a conversion between integer types. |
To allow users to determine thread in a multithreaded application
corresponds to the "Thread X of Y" number displayed in the live view,
include the thread name that we collect in the TUI.
Closes: #561