From d82e4acefda81998d41739018b7f0700e30f09b5 Mon Sep 17 00:00:00 2001 From: Aaditya Sondhi <20070511+aadityasondhi@users.noreply.github.com> Date: Fri, 13 Jan 2023 15:32:40 -0500 Subject: [PATCH 1/2] schedulerlatency: implement linear interpolation for percentiles 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 --- pkg/util/schedulerlatency/sampler.go | 45 +++++++++++------ .../scheduler_latency_test.go | 50 ++++++++++++++++--- 2 files changed, 75 insertions(+), 20 deletions(-) diff --git a/pkg/util/schedulerlatency/sampler.go b/pkg/util/schedulerlatency/sampler.go index d45e52f2bc46..9aa92b375637 100644 --- a/pkg/util/schedulerlatency/sampler.go +++ b/pkg/util/schedulerlatency/sampler.go @@ -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 @@ -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 } diff --git a/pkg/util/schedulerlatency/scheduler_latency_test.go b/pkg/util/schedulerlatency/scheduler_latency_test.go index e46c5a4c3593..1ec15ed46987 100644 --- a/pkg/util/schedulerlatency/scheduler_latency_test.go +++ b/pkg/util/schedulerlatency/scheduler_latency_test.go @@ -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 } { @@ -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 │ ┌───┐ From 77732e106d058d070da5958ef51544e3539b7307 Mon Sep 17 00:00:00 2001 From: Andy Yang Date: Fri, 13 Jan 2023 14:54:59 -0500 Subject: [PATCH 2/2] upgrades: modify InjectLegacyTable to handle dynamically assigned IDs 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. Release note: None --- pkg/upgrade/upgrades/BUILD.bazel | 1 + pkg/upgrade/upgrades/helpers_test.go | 27 ++++++++++++++++++++++----- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/pkg/upgrade/upgrades/BUILD.bazel b/pkg/upgrade/upgrades/BUILD.bazel index 0a2cbb247ade..6f3231c3ab55 100644 --- a/pkg/upgrade/upgrades/BUILD.bazel +++ b/pkg/upgrade/upgrades/BUILD.bazel @@ -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", diff --git a/pkg/upgrade/upgrades/helpers_test.go b/pkg/upgrade/upgrades/helpers_test.go index 1a3e86d67346..e5be578d47f9 100644 --- a/pkg/upgrade/upgrades/helpers_test.go +++ b/pkg/upgrade/upgrades/helpers_test.go @@ -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" @@ -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 }