-
Notifications
You must be signed in to change notification settings - Fork 4k
obs, kv, sql: add workloadID to profile tags #158931
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
base: master
Are you sure you want to change the base?
Conversation
|
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. |
tbg
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.
Only a small suggestion about the replica-level region tags. LGTM otherwise and thanks for doing this!
@tbg reviewed 16 of 16 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @alyshanjahani-crl, @dhartunian, and @jeffswenson)
pkg/obs/workloadid/workloadid.go line 52 at r1 (raw file):
// context by ContextWithWorkloadID. func WorkloadIDFromContext(ctx context.Context) WorkloadID { r := ctxutil.FastValue(ctx, workloadIDKey)
cockroach/pkg/util/ctxutil/fast_value.go
Lines 41 to 42 in 7e69519
| // The given value will be returned by FastValue(ctx, key). The key must have | |
| // been generated using RegisterFastValueKey(). |
Are you calling RegisterFastValueKey?
pkg/kv/kvserver/replica_send.go line 151 at r1 (raw file):
// This label is set in conn_executor_exec if tracing is active if l == workloadid.ProfileTag { regionTag = fmt.Sprintf("w:%s r:%s", ba.ProfileLabels[i+1], regionTag)
It's probably potato potahto given our limited use of this information, but by flattening the tag x rangeID cross-product into a string, we make it harder to aggregate stats by workloadID or region alone (gotraceui can give you histograms by region name for example). Have you considered instead starting nested regions? That would also avoid allocs related to Sprintf. It will also avoid having to embed more cross-product strings in the exectrace, which could somewhat inflate its volume.
Some relevant AI analysis below. I think even if we're only doing this during active exectrace I see no reason why this should need to allocate on all but the first calls, so getting rid of the Sprintf makes sense to me.
How trace.WithRegion Stores Region Strings
Region strings are deduplicated in Go execution traces. Each unique region type string is stored only once per trace generation, and subsequent uses reference the existing string ID.
How It Works
WithRegion(ctx, regionType, fn)calls the runtime'suserRegion()function- The runtime calls
tl.string(name)which uses a string table (traceStringTable) - The string table uses a lock-free hash-trie (
traceMap) to deduplicate:
// src/runtime/tracestring.go
func (t *traceStringTable) put(gen uintptr, s string) uint64 {
ss := stringStructOf(&s)
id, added := t.tab.put(ss.str, uintptr(ss.len))
if added {
// Only write the string if it's new
systemstack(func() {
t.writeString(gen, id, s)
})
}
return id
}- The
traceMap.put()checks for existing entries using hash + byte equality:
// src/runtime/tracemap.go
if n.hash == hash && uintptr(len(n.data)) == size {
if memequal(unsafe.Pointer(&n.data[0]), data, size) {
return n.id, false // Already exists, return existing ID
}
}Result
- First call with a region string: string bytes are written to trace, assigned unique ID
- Subsequent calls with the same string: only the ID is recorded (no string copy)
So calling trace.WithRegion(ctx, "myRegion", fn) a million times stores "myRegion" only once in the trace output.
Note
The string table is per-generation (stringTab[gen%2]), so deduplication happens within each trace generation. The trace format stores strings in a dictionary section (traceEvStrings/traceEvString events) and references them by ID in the actual region events (traceEvUserRegionBegin/traceEvUserRegionEnd).
We introduce `pkg/obs/workloadid` which defines a commond workloadID definition, subject to extensive future modification, that we can use to tag CPU profiles and execution trace regions. Currently, the workloadID is either a static value, or a statement fingerprint ID. Where appropriate, we've tagged CPU profiles with a workload identifier. When tracing is enabled, on a request, we will attempt to include the workloadID into the region tag in the execution trace. This path is a bit perf sensitive so we only want to incur the cost of allocating the extra string when we're already paying the cost of tracing. It is expected that the combination of statementb diagnostic bundle capture and execution tracing can then be cross-referenced. Epic: CRDB-55080 Resolves: CRDB-55928 Resolves: CRDB-55924 Release note: None
0763b6b to
6a2ca47
Compare
dhartunian
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.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @alyshanjahani-crl, @jeffswenson, and @tbg)
pkg/kv/kvserver/replica_send.go line 151 at r1 (raw file):
Previously, tbg (Tobias Grieger) wrote…
It's probably potato potahto given our limited use of this information, but by flattening the
tag x rangeIDcross-product into a string, we make it harder to aggregate stats by workloadID or region alone (gotraceui can give you histograms by region name for example). Have you considered instead starting nested regions? That would also avoid allocs related toSprintf. It will also avoid having to embed more cross-product strings in the exectrace, which could somewhat inflate its volume.Some relevant AI analysis below. I think even if we're only doing this during active exectrace I see no reason why this should need to allocate on all but the first calls, so getting rid of the Sprintf makes sense to me.
How
trace.WithRegionStores Region StringsRegion strings are deduplicated in Go execution traces. Each unique region type string is stored only once per trace generation, and subsequent uses reference the existing string ID.
How It Works
WithRegion(ctx, regionType, fn)calls the runtime'suserRegion()function- The runtime calls
tl.string(name)which uses a string table (traceStringTable)- The string table uses a lock-free hash-trie (
traceMap) to deduplicate:// src/runtime/tracestring.go func (t *traceStringTable) put(gen uintptr, s string) uint64 { ss := stringStructOf(&s) id, added := t.tab.put(ss.str, uintptr(ss.len)) if added { // Only write the string if it's new systemstack(func() { t.writeString(gen, id, s) }) } return id }
- The
traceMap.put()checks for existing entries using hash + byte equality:// src/runtime/tracemap.go if n.hash == hash && uintptr(len(n.data)) == size { if memequal(unsafe.Pointer(&n.data[0]), data, size) { return n.id, false // Already exists, return existing ID } }Result
- First call with a region string: string bytes are written to trace, assigned unique ID
- Subsequent calls with the same string: only the ID is recorded (no string copy)
So calling
trace.WithRegion(ctx, "myRegion", fn)a million times stores"myRegion"only once in the trace output.Note
The string table is per-generation (
stringTab[gen%2]), so deduplication happens within each trace generation. The trace format stores strings in a dictionary section (traceEvStrings/traceEvStringevents) and references them by ID in the actual region events (traceEvUserRegionBegin/traceEvUserRegionEnd).
Thanks for the detail here. I split up into two regions.
pkg/obs/workloadid/workloadid.go line 52 at r1 (raw file):
Previously, tbg (Tobias Grieger) wrote…
cockroach/pkg/util/ctxutil/fast_value.go
Lines 41 to 42 in 7e69519
// The given value will be returned by FastValue(ctx, key). The key must have // been generated using RegisterFastValueKey(). Are you calling RegisterFastValueKey?
Yes right above on line 40.
We introduce
pkg/obs/workloadidwhich defines a commond workloadID definition, subject to extensive future modification, that we can use to tag CPU profiles and execution trace regions.Currently, the workloadID is either a static value, or a statement fingerprint ID.
Where appropriate, we've tagged CPU profiles with a workload identifier.
When tracing is enabled, on a request, we will attempt to include the workloadID into the region tag in the execution trace. This path is a bit perf sensitive so we only want to incur the cost of allocating the extra string when we're already paying the cost of tracing. It is expected that the combination of statementb diagnostic bundle capture and execution tracing can then be cross-referenced.
Epic: CRDB-55080
Resolves: CRDB-55928
Resolves: CRDB-55924
Release note: None