-
Notifications
You must be signed in to change notification settings - Fork 5
feat: add lock name to SQL to trace the source #27
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
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Uh oh!
There was an error while loading. Please reload this page.
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.
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
SELECTstatements 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Uh oh!
There was an error while loading. Please reload this page.
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.
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.
Uh oh!
There was an error while loading. Please reload this page.
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.
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