-
Notifications
You must be signed in to change notification settings - Fork 40
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
Add DTrace scripts to Nexus zone #7244
Conversation
bnaecker
commented
Dec 12, 2024
- Adds a script for tracing all transactions run by Nexus, including their overall latency and the number of statements in each one.
- Move all existing scripts to a Nexus subdirectory, and then include that whole directory in the Omicron zone for Nexus itself.
- Closes Would like D scripts for tracing transactions in Nexus #7224
For a quick confirmation that these scripts are actually in the Nexus zone tarball, I built everything using
Most of these scripts have been around for a long time, I'm just including them in the zone image now. I've also added one new script
So this shows us the count of statements in each transaction when it's either committed or rolled back, along with the connection ID and total duration of the transaction itself. This would have certainly helped in the investigation of #7208, where many transactions were both long and including a huge number of statements. I'm sure we'll want more scripts in the long run, though this is a good start. |
We have a pile of crucible related DTrace scripts include in all global zones. Should we define (and have all of us use) a common top level directory to put all these? |
That sounds like a good idea! I didn't realize the Crucible scripts were already in the GZ. What are the tradeoffs to putting them there as opposed to the Crucible zone? I added these to the Nexus zone itself, but am happy to move them if that makes sense. A path structure like |
There was a time when I could only run DTrace in the global zone, but I don't think that is still true. |
Thanks @leftwo that makes sense. We can definitely run DTrace from a non-global zone now. I don't really see a downside to putting them in the GZ, so I'll go ahead and do that instead. Does |
Seems fine to me. We (I) can move the crucible ones in a different PR. |
One risk of putting these in the GZ is that they can be mismatched with the zone (if the probes and scripts change over time). That may be uncommon enough that we don't care but I figured I'd mention it. |
That's a good point, and yes, it depends on what the probes are probing. |
Good point @davepacheco, thanks for bringing that up. The probes haven't changed much in a while, but I'm also hesitant to introduce more coupling than we need to. There's always a chance that the scripts are out of date even in the zone, since nothing really checks that at packaging time. Still, given that you can always get into the Nexus zone from the GZ, but not vice-versa, it seems safer to deploy these as part of that zone. |
I'm going to wait for an update to |
Ok, the scripts are updated with the new version of |
Oops, didn't mean to request review yet. I think we'll rework this to use the retry-wrapper probes Sean added in #7248. |
- Adds a "NonRetryHelper" struct to help instrument non-retryable transactions - Adds `transaction__start` and `transaction__done` probes which wrap our retryable (and now, non-retryable transactions) Intended to supplement #7244 , and provide transaction names
- Adds a script for tracing all transactions run by Nexus, including their overall latency and the number of statements in each one. - Move all existing scripts to a Nexus subdirectory, and then include that whole directory in the Omicron zone for Nexus itself. - Closes #7224
33317df
to
9853e1b
Compare
Ok, I've updated this to use the new probes @smklein added in #7248. Here's what we get, running Omicron locally on my Helios machine:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ship it!
Did this change not make it into the final PR? |
@leftwo Right now, I'd prefer to keep these scripts in the zone so we're less likely to end up with a mismatch between the scripts in the GZ and the actual probes in the zone itself. I think putting the scripts that are in the GZ in that path structure makes a lot of sense. |
Yeah, sorry, I was not specific. Having them zone only is fine. |
As of 5e90bac, the scripts are now here:
|
Ah, I didn't understand that. In the latest commit, I put them in the same location as the binary they're intended to trace, |
Yeah, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stamp of rubber has been applied!