feat: add lock name to SQL to trace the source#27
Conversation
|
|
||
| def acquire_lock | ||
| connection.execute("select pg_advisory_xact_lock(#{connection.quote(db_lock_name)})") | ||
| connection.execute("/* lock:#{lock_name} */ SELECT pg_advisory_xact_lock(#{connection.quote(db_lock_name)})") |
There was a problem hiding this comment.
Does this break performance insights? I vaguely remember some issue where high cardinality SQL comments would break the ability for queries to be grouped within the performance insights dashboard.
There was a problem hiding this comment.
good question!
so, i think it won't break it per se, but it will result in giving us a different entry per lock name instead of one entry for all calls into this codepath.
i think that's precisely what we want in this case.
the instance you're thinking of was putting a uuid in the comment so we could identify exactly which request or job was causing a query to happen and we pivoted from that to just tagging with the class of the request or job so that they'd group up in a useful way.
does that sound right @matsuimp?
There was a problem hiding this comment.
the instance you're thinking of was putting a uuid in the comment so we could identify exactly which request or job was causing a query to happen and we pivoted from that to just tagging with the class of the request or job so that they'd group up in a useful way.
yup, that sounds right to me. we we're doing /* JobName-<UUID> */ in the queries. i take it we don't have high number of lock names in practice? i.e, this won't be an account id in practice?
There was a problem hiding this comment.
We don't use record IDs in lock names?
I believe the receiving end of a first-time record sync (where the remote record is the source of truth) might need such a lock.
There was a problem hiding this comment.
You know what? You're absolutely right, we also have locks names that include identifiers.
So yes, this is potentially going to create a lot of buckets and in some cases more than we want. Perhaps we should try a different approach.
Need to noodle on this!
There was a problem hiding this comment.
If we wanted to enable it for longer, we could also have it support a list of "known" lock names to match against, and if there is no match it would not append the name.
There was a problem hiding this comment.
comment: Thinking about this a bit more, I don't think that we even need to worry about the potential of "creating a lot of buckets". I don't think we should pursue my env var idea.
Consider what DB Insights is currently reporting:
The above report shows a 1 hr timespan. If I change it to a 4 week timespan, it's the same 4 queries. It's very consistent.
So then the worst case is we'd see these 4 queries expanded out into 4 separate line items in DB Insights. I would argue this is an improvement because it more accurately reflects the state of the system. We will likely need to solve the problems with the 4 queries in 4 completely different ways. The fact they are grouped into a single line item due to a similar shape is suboptimal, similar to if we had grouped all SELECT statements into a single line item because of the similar shape.
We currently have 4 unrelated queues that happen to be using the same library getting lumped in together, and we should have them split up. It's only an accident of implementation that they are grouped together.
There was a problem hiding this comment.
Seems fine to me then.
We may want the ability to turn this on/off without having to revert the version of the library.
Do we feel strongly that we don't need any config for this? I'm fine with shipping it always on and coming back here to add a config option if it's an issue.
There was a problem hiding this comment.
comment: Given I can't point to any specific cases where it needs to be configurable, I'd err on the side of not adding the code/functionality. I usually prefer to have at least one concrete use case before adding to the system because the number of "what if" scenarios we can imagine is unbounded and it would be a bad precedence for us to chase all of them down without any evidence.
Also, given that reverting the change is trivial I'd not bother to add this since the cost to add it and make sure the config changes are deployed would easily exceed 10 to 50x the cost of a revert.
There was a problem hiding this comment.
comment: Just following up after deploying this, and it seems as if Database Insights is still smart enough to understand the commenting pattern we're using and group things together
|
comment: I don't have access to approve this PR but I have no disagreements with the current code given the conversation thus far (I am biased of course, so someone with access should approve). |
4 participants
rebasing and tweaking #25 by @dkubb
/no-platform