sql: include regions information into the sampled query telemetry#86829
sql: include regions information into the sampled query telemetry#86829craig[bot] merged 2 commits intocockroachdb:masterfrom
Conversation
|
This change is |
b7f7487 to
916c1b0
Compare
rytaft
left a comment
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, 10 of 10 files at r2, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @THardy98)
This field was never actually populated nor used since it was introduced in 9f716a7. Release justification: low-risk cleanup. Release note: None
7fef74f to
92a5003
Compare
92a5003 to
68a7bf1
Compare
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewable status:
docs/generated/eventlog.md line 2607 at r4 (raw file):
| `ZigZagJoinCount` | The number of zig zag joins in the query plan. | no | | `ContentionNanos` | The duration of time in nanoseconds that the query experienced contention. | no | | `Regions` | The regions of the nodes where SQL processors ran. | no |
I'm assuming that the regions information is not sensitive, does that sound right? cc @kevin-v-ngo
68a7bf1 to
f8ebd20
Compare
This commit adds the regions (where SQL processors where planned for the query) to the sampled query telemetry. This required a couple of minor changes to derive the regions information stored in the instrumentation helper earlier (before the logging takes place). Release justification: low-risk improvement. Release note (sql change): The structured payloads used for telemetry logs now include the new `Regions` field which indicates the regions of the nodes where SQL processing ran for the query.
f8ebd20 to
73e9ff0
Compare
|
TFTR! bors r+ |
|
Build succeeded: |
|
Can we backport this to 22.1? |
This would require pulling in some of the changes made in #84718 (in particular, we'd need to get the trace recording one more time, before logging the telemetry). Is the backport desirable? |
|
cc @vy-ton @kevin-v-ngo @awoods187 see @yuzefovich's question above. Not sure how important it is to get region telemetry from 22.1. (Otherwise this will just be included in 22.2) |
|
If possible, I'd like to see this backported to 22.1. |
execstats: remove unused regions field
This field was never actually populated nor used since it was introduced
in 9f716a7.
Release justification: low-risk cleanup.
Release note: None
sql: include regions information into the sampled query telemetry
This commit adds the regions (where SQL processors where planned for the
query) to the sampled query telemetry. This required a couple of minor
changes to derive the regions information stored in the instrumentation
helper earlier (before the logging takes place).
Fixes: #85427.
Release justification: low-risk improvement.
Release note (sql change): The structured payloads used for telemetry
logs now include the new
Regionsfield which indicates the regions ofthe nodes where SQL processing ran for the query.