-
Notifications
You must be signed in to change notification settings - Fork 4k
release-26.1: sql: attach opName string as pprof label for internal queries #160034
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
release-26.1: sql: attach opName string as pprof label for internal queries #160034
Conversation
ce064ee to
30573ac
Compare
|
Thanks for opening a backport. Before merging, please confirm that the change does not break backwards compatibility and otherwise complies with the backport policy. Include a brief release justification in the PR description explaining why the backport is appropriate. All backports must be reviewed by the TL for the owning area. While the stricter LTS policy does not yet apply, please exercise judgment and consider gating non-critical changes behind a disabled-by-default feature flag when appropriate. |
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
| "stmt.no.constants", stmtNoConstants, | ||
| } | ||
| if opName, ok := GetInternalOpName(ctx); ok { | ||
| labels = append(labels, "opname", opName) |
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.
I realized we can pre-allocate and extra slot in the []string to avoid the extra allocation on append, but probably not worth it given that is is within the profiling code path only.
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.
Yeah, I also thought about this initially and had the same reasoning (slow path anyway) for not doing that. But given you mentioned it too and it's easy enough to avoid, done.
Every query that we run via the internal executor comes with "opName" string that is a short description of the query. We now will attach that string as a pprof label that can then be propagated into stack traces. Release note: None
30573ac to
99ec795
Compare
michae2
left a comment
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.
@michae2 reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @mgartner).
|
TFTRs! |
Backport 1/1 commits from #159964 on behalf of @yuzefovich.
Every query that we run via the internal executor comes with "opName" string that is a short description of the query. We now will attach that string as a pprof label that can then be propagated into stack traces.
Epic: None
Release note: None
Release justification: low-risk debugging improvement.