keyvisualizer: pre-aggregate ranges #98707
Conversation
|
This change is |
There was a problem hiding this comment.
Some small comments
Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @zachlite)
pkg/keyvisualizer/spanstatsconsumer/span_stats_consumer.go line 91 at r1 (raw file):
} // combineFactor is factor that boundaries length is reduced by. For example,
nit: is a factor
pkg/keyvisualizer/spanstatsconsumer/span_stats_consumer.go line 100 at r1 (raw file):
// Iterate through boundaries, incrementing by combineFactor. for i := 0; i < combinedLength; i++ { startSpan := boundaries[i*combineFactor]
There is no situation where we will end up with an index out of bound error right?
pkg/ui/workspaces/db-console/src/views/keyVisualizer/index.tsx line 170 at r1 (raw file):
return ( <div style={{ position: "relative" }}> <KeyVisualizerTimeWindow />
If retention period is now only 7 days do we want to remove the two week option?
zachlite
force-pushed
the
230315.keyvis-preaggregation
branch
from
c0d612b to
e081b2d
Compare
Previously, there was no bound on the number of ranges that could be propagated to the collectors. After collection, data was downsampled using a simple heurstic to decide if a bucket was worth keeping or if it should be aggregated with its neighbor. In this commit, I've introduced a function, `maybeAggregateBoundaries`, to prevent more than `keyvisualizer.max_buckets` from being propagated to collectors. This pre-aggregation takes the place of the post-collection downsampling. For the first stable release of the key visualizer, I am intentionally sacrificing dynamic resolution and prioritizing boundary stability instead. This trade-off means that the key visualizer will demand less network, memory, and storage resources from the cluster while operating. Additionally, this PR drops the sample retention time from 14 days to 7 days, and ensures that `keyvisualizer.max_buckets` is bounded between [1, 1024]. Resolves: cockroachdb#96740 Epic: None Release note: None
zachlite
force-pushed
the
230315.keyvis-preaggregation
branch
from
e081b2d to
f8ba475
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/keyvisualizer/spanstatsconsumer/span_stats_consumer.go line 91 at r1 (raw file):
Previously, Santamaura (Alex Santamaura) wrote…
nit: is a factor
fixed.
pkg/keyvisualizer/spanstatsconsumer/span_stats_consumer.go line 100 at r1 (raw file):
Previously, Santamaura (Alex Santamaura) wrote…
There is no situation where we will end up with an index out of bound error right?
Correct, but I'll put a check for good measure.
pkg/ui/workspaces/db-console/src/views/keyVisualizer/index.tsx line 170 at r1 (raw file):
Previously, Santamaura (Alex Santamaura) wrote…
If retention period is now only 7 days do we want to remove the two week option?
No we don't. thanks.
There was a problem hiding this comment.
addressed!
Reviewable status:
There was a problem hiding this comment.
Reviewed 3 of 4 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status:
|
bors r+ |
|
Build failed: |
|
bors r+ |
|
Build failed (retrying...): |
|
Build succeeded: |
Previously, there was no bound on the number of ranges that could be propagated to the collectors. After collection, data was downsampled using a simple heurstic to decide if a bucket was worth keeping or if it should be aggregated with its neighbor.
In this commit, I've introduced a function,
maybeAggregateBoundaries, to prevent more thankeyvisualizer.max_bucketsfrom being propagated to collectors. This pre-aggregation takes the place of the post-collection downsampling. For the first stable release of the key visualizer, I am intentionally sacrificing dynamic resolution and prioritizing boundary stability instead. This trade-off means that the key visualizer will demand less network, memory, and storage resources from the cluster while operating.Additionally, this PR drops the sample retention time from 14 days to 7 days, and ensures that
keyvisualizer.max_bucketsis bounded between [1, 1024].Resolves: #96740
Epic: None
Release note: None