Skip to content

Commit

Permalink
Merge #94193 #95231
Browse files Browse the repository at this point in the history 
94193: schedulerlatency: implement linear interpolation for percentiles r=aadityasondhi a=aadityasondhi

This patch improves the percentile calculation by using linear interpolation between bucket boundaries instead of using the mid-point. The patch also adds tests to compare values of the scheduler histograms to those found in util/metric.go which are based on the prometheus implementation to maintain consistency among metrics.

Fixes #89829

Release note: None

95231: upgrades: modify InjectLegacyTable to handle dynamically assigned IDs r=postamar a=andyyang890

This patch modifies the `InjectLegacyTable` function, which is used by
upgrade tests to inject an old version of a table descriptor, to handle
the case where the table has a dynamically assigned ID.

Epic: None

Release note: None

Co-authored-by: Aaditya Sondhi <[email protected]>
Co-authored-by: Andy Yang <[email protected]>
  • Loading branch information
@aadityasondhi @andyyang890
3 people committed Jan 19, 2023
3 parents 38d207a + d82e4ac + 77732e1 commit 942b55e
Show file tree
Hide file tree
Showing 4 changed files with 98 additions and 25 deletions.
1 change: 1 addition & 0 deletions pkg/upgrade/upgrades/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ go_test(
"//pkg/sql/catalog/descbuilder",
"//pkg/sql/catalog/descpb",
"//pkg/sql/catalog/descs",
"//pkg/sql/catalog/schemadesc",
"//pkg/sql/catalog/schematelemetry/schematelemetrycontroller",
"//pkg/sql/catalog/systemschema",
"//pkg/sql/catalog/tabledesc",
Expand Down
27 changes: 22 additions & 5 deletions pkg/upgrade/upgrades/helpers_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/catalog"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/schemadesc"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -76,12 +78,27 @@ func InjectLegacyTable(
err := s.InternalDB().(descs.DB).DescsTxn(ctx, func(
ctx context.Context, txn descs.Txn,
) error {
id := table.GetID()
tab, err := txn.Descriptors().MutableByID(txn.KV()).Table(ctx, id)
if err != nil {
return err
deprecatedDesc := getDeprecatedDescriptor()
var tab *tabledesc.Mutable
switch id := table.GetID(); id {
// If the table descriptor does not have a valid ID, it must be a system
// table with a dynamically-allocated ID.
case descpb.InvalidID:
var err error
tab, err = txn.Descriptors().MutableByName(txn.KV()).Table(ctx,
systemschema.SystemDB, schemadesc.GetPublicSchema(), table.GetName())
if err != nil {
return err
}
deprecatedDesc.ID = tab.GetID()
default:
var err error
tab, err = txn.Descriptors().MutableByID(txn.KV()).Table(ctx, id)
if err != nil {
return err
}
}
builder := tabledesc.NewBuilder(getDeprecatedDescriptor())
builder := tabledesc.NewBuilder(deprecatedDesc)
if err := builder.RunPostDeserializationChanges(); err != nil {
return err
}
Expand Down
45 changes: 31 additions & 14 deletions pkg/util/schedulerlatency/sampler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -298,11 +298,33 @@ func percentile(h *metrics.Float64Histogram, p float64) float64 {
total += h.Counts[i]
}

// Iterate backwards (we're optimizing for higher percentiles) until we find
// the right bucket.
// Linear approximation of the target value corresponding to percentile, p:
//
// Goal: We want to linear approximate the "p * total"-th value from the histogram.
// - p * total is the ordinal rank (or simply, rank) of the target value
// within the histogram bounds.
//
// Step 1: Find the bucket[i] which contains the target value.
// - This will be the largest bucket[i] where the left-bound cumulative
// count of bucket[i] <= p*total.
//
// Step 2: Determine the rank within the bucket (subset of histogram).
// - Since the bucket is a subset of the original data set, we can simply
// subtract left-bound cumulative to get the "subset_rank".
//
// Step 3: Use the "subset_rank" to interpolate the target value.
// - So we want to interpolate the "subset_rank"-th value of the subset_count
// that lies between [bucket_start, bucket_end). This is:
// bucket_start + (bucket_start - bucket_end) * linear_interpolator
// Where linear_interpolator = subset_rank / subset_count

// (Step 1) Iterate backwards (we're optimizing for higher percentiles) until
// we find the first bucket for which total-cumulative <= rank, which will be
// by design the largest bucket that meets that condition.
var cumulative uint64 // cumulative count of all buckets we've iterated through
var start, end float64 // start and end of current bucket
for i := len(h.Counts) - 1; i >= 0; i-- {
var i int // index of current bucket
for i = len(h.Counts) - 1; i >= 0; i-- {
start, end = h.Buckets[i], h.Buckets[i+1]
if i == 0 && math.IsInf(h.Buckets[0], -1) { // -Inf
// Buckets[0] is permitted to have -Inf; avoid interpolating with
Expand Down Expand Up @@ -332,15 +354,10 @@ func percentile(h *metrics.Float64Histogram, p float64) float64 {
}
}

return (start + end) / 2 // grab the mid-point within the bucket

// TODO(irfansharif): Implement proper linear interpolation instead and then
// write test comparing against it against metric.ValueAtQuantileWindowed.
// If pmax, we'll return the bucket max. If pmin, we'll return the bucket
// min. If the 90th and 100th percentile values lie within some bucket, and
// we're looking for 99.9th percentile, we'll interpolate to the 99% point
// between bucket start and end. Because we're doing this naive mid-point
// thing, it makes for a confusing difference when comparing the p99
// computed off of go.scheduler_latency vs.
// admission.scheduler_latency_listener.p99_nanos.
// (Step 2) Find the target rank within bucket boundaries.
subsetRank := float64(total)*p - float64(total-cumulative)

// (Step 3) Using rank and the percentile to calculate the approximated value.
subsetPercentile := subsetRank / float64(h.Counts[i])
return start + (end-start)*subsetPercentile
}
50 changes: 44 additions & 6 deletions pkg/util/schedulerlatency/scheduler_latency_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,12 +119,12 @@ func TestComputeSchedulerPercentile(t *testing.T) {
Buckets: []float64{0, 10, 20, 30, 40, 50, 60, 70, 80, 90, 100, 110, 120, 130},
}

require.Equal(t, 95.0, percentile(&hist, 1.00)) // pmax
require.Equal(t, 05.0, percentile(&hist, 0.00)) // pmin
require.Equal(t, 45.0, percentile(&hist, 0.50)) // p50
require.Equal(t, 75.0, percentile(&hist, 0.75)) // p75
require.Equal(t, 95.0, percentile(&hist, 0.90)) // p90
require.Equal(t, 95.0, percentile(&hist, 0.99)) // p99
require.InDelta(t, 100.0, percentile(&hist, 1.00), 0.001) // pmax
require.InDelta(t, 0.0, percentile(&hist, 0.00), 0.001) // pmin
require.InDelta(t, 49.375, percentile(&hist, 0.50), 0.001) // p50
require.InDelta(t, 70.625, percentile(&hist, 0.75), 0.001) // p75
require.InDelta(t, 90.600, percentile(&hist, 0.90), 0.001) // p90
require.InDelta(t, 99.060, percentile(&hist, 0.99), 0.001) // p99
}

{
Expand All @@ -151,6 +151,44 @@ func TestComputeSchedulerPercentile(t *testing.T) {
}
}

func TestComputeSchedulerPercentileAgainstPrometheus(t *testing.T) {
{
// ▲
// 8 │ ┌───┐
// 7 │ ┌───┤ │
// 6 │ │ │ ├───┐
// 5 │ ┌───┤ │ │ ├───┐ ┌───┐
// 4 │ │ │ │ │ │ ├───┐ │ │
// 3 │ ┌───┤ │ │ │ │ │ ├───┤ │
// 2 │ │ │ │ │ │ │ │ │ │ │
// 1 ├───┤ │ │ │ │ │ │ │ │ │
// └───┴───┴───┴───┴───┴───┴───┴───┴───┴───┴─────────────▶
// 10 20 30 40 50 60 70 80 90 100 110 120 130
hist := metrics.Float64Histogram{
Counts: []uint64{1, 3, 5, 7, 8, 6, 5, 4, 3, 5, 0, 0, 0},
Buckets: []float64{0, 10, 20, 30, 40, 50, 60, 70, 80, 90, 100, 110, 120, 130},
}

// Compare values against metric.Histogram (prometheus-based implementation)
promhist := metric.NewHistogram(metric.Metadata{}, time.Hour, hist.Buckets)
for i := 0; i < len(hist.Counts); i++ {
for j := 0; j < int(hist.Counts[i]); j++ {
// Since the scheduler buckets are non-inclusive of Upper Bound and prometheus
// buckets are inclusive, we use i+1 below to create an identical histogram in
// prometheus.
promhist.RecordValue(int64(hist.Buckets[i+1]))
}
}

require.InDelta(t, promhist.ValueAtQuantileWindowed(100), percentile(&hist, 1.00), 1) // pmax
require.InDelta(t, promhist.ValueAtQuantileWindowed(0), percentile(&hist, 0.00), 1) // pmin
require.InDelta(t, promhist.ValueAtQuantileWindowed(50), percentile(&hist, 0.50), 1) // p50
require.InDelta(t, promhist.ValueAtQuantileWindowed(75), percentile(&hist, 0.75), 1) // p75
require.InDelta(t, promhist.ValueAtQuantileWindowed(90), percentile(&hist, 0.90), 1) // p90
require.InDelta(t, promhist.ValueAtQuantileWindowed(99), percentile(&hist, 0.99), 1) // p99
}
}

func TestSubtractHistograms(t *testing.T) {
// ▲
// 8 │ ┌───┐
Expand Down

0 comments on commit 942b55e

Please sign in to comment.