From 6402a2275fe9cad3441d2d3bca8e1c5cb5d035ea Mon Sep 17 00:00:00 2001 From: Samantha Frank Date: Fri, 20 Dec 2024 08:44:08 -0500 Subject: [PATCH] ratelimits: Remove a metric and some labels that we're not finding useful (#7902) --- ratelimits/limit.go | 21 ++++++++------------- ratelimits/limiter.go | 21 ++++----------------- ratelimits/limiter_test.go | 13 ------------- ratelimits/source_redis.go | 16 ---------------- ratelimits/transaction.go | 4 ++-- ratelimits/transaction_test.go | 24 ++++++++++++------------ 6 files changed, 26 insertions(+), 73 deletions(-) diff --git a/ratelimits/limit.go b/ratelimits/limit.go index 16dc65ac962..72fc3a1a555 100644 --- a/ratelimits/limit.go +++ b/ratelimits/limit.go @@ -69,13 +69,8 @@ type limit struct { // precomputed to avoid doing the same calculation on every request. burstOffset int64 - // overrideKey is the key used to look up this limit in the overrides map. - overrideKey string -} - -// isOverride returns true if the limit is an override. -func (l *limit) isOverride() bool { - return l.overrideKey != "" + // isOverride is true if the limit is an override. + isOverride bool } // precompute calculates the emissionInterval and burstOffset for the limit. @@ -178,11 +173,13 @@ func parseOverrideLimits(newOverridesYAML overridesYAML) (limits, error) { } lim := &limit{ - burst: v.Burst, - count: v.Count, - period: v.Period, - name: name, + burst: v.Burst, + count: v.Count, + period: v.Period, + name: name, + isOverride: true, } + lim.precompute() err := validateLimit(lim) if err != nil { @@ -196,14 +193,12 @@ func parseOverrideLimits(newOverridesYAML overridesYAML) (limits, error) { return nil, fmt.Errorf( "validating name %s and id %q for override limit %q: %w", name, id, k, err) } - lim.overrideKey = joinWithColon(name.EnumString(), id) if name == CertificatesPerFQDNSet { // FQDNSet hashes are not a nice thing to ask for in a // config file, so we allow the user to specify a // comma-separated list of FQDNs and compute the hash here. id = fmt.Sprintf("%x", core.HashNames(strings.Split(id, ","))) } - lim.precompute() parsed[joinWithColon(name.EnumString(), id)] = lim } } diff --git a/ratelimits/limiter.go b/ratelimits/limiter.go index ef119d1819a..f886cdc482a 100644 --- a/ratelimits/limiter.go +++ b/ratelimits/limiter.go @@ -37,8 +37,7 @@ type Limiter struct { source Source clk clock.Clock - spendLatency *prometheus.HistogramVec - overrideUsageGauge *prometheus.GaugeVec + spendLatency *prometheus.HistogramVec } // NewLimiter returns a new *Limiter. The provided source must be safe for @@ -52,17 +51,10 @@ func NewLimiter(clk clock.Clock, source Source, stats prometheus.Registerer) (*L }, []string{"limit", "decision"}) stats.MustRegister(spendLatency) - overrideUsageGauge := prometheus.NewGaugeVec(prometheus.GaugeOpts{ - Name: "ratelimits_override_usage", - Help: "Proportion of override limit used, by limit name and bucket key.", - }, []string{"limit", "bucket_key"}) - stats.MustRegister(overrideUsageGauge) - return &Limiter{ - source: source, - clk: clk, - spendLatency: spendLatency, - overrideUsageGauge: overrideUsageGauge, + source: source, + clk: clk, + spendLatency: spendLatency, }, nil } @@ -284,11 +276,6 @@ func (l *Limiter) BatchSpend(ctx context.Context, txns []Transaction) (*Decision storedTAT, bucketExists := tats[txn.bucketKey] d := maybeSpend(l.clk, txn, storedTAT) - if txn.limit.isOverride() { - utilization := float64(txn.limit.burst-d.remaining) / float64(txn.limit.burst) - l.overrideUsageGauge.WithLabelValues(txn.limit.name.String(), txn.limit.overrideKey).Set(utilization) - } - if d.allowed && (storedTAT != d.newTAT) && txn.spend { if !bucketExists { newBuckets[txn.bucketKey] = d.newTAT diff --git a/ratelimits/limiter_test.go b/ratelimits/limiter_test.go index 902f4c13435..5ccb7dfa0a9 100644 --- a/ratelimits/limiter_test.go +++ b/ratelimits/limiter_test.go @@ -8,7 +8,6 @@ import ( "time" "github.com/jmhodges/clock" - "github.com/prometheus/client_golang/prometheus" "github.com/letsencrypt/boulder/config" berrors "github.com/letsencrypt/boulder/errors" @@ -60,12 +59,6 @@ func TestLimiter_CheckWithLimitOverrides(t *testing.T) { testCtx, limiters, txnBuilder, clk, testIP := setup(t) for name, l := range limiters { t.Run(name, func(t *testing.T) { - // Verify our overrideUsageGauge is being set correctly. 0.0 == 0% - // of the bucket has been consumed. - test.AssertMetricWithLabelsEquals(t, l.overrideUsageGauge, prometheus.Labels{ - "limit": NewRegistrationsPerIPAddress.String(), - "bucket_key": joinWithColon(NewRegistrationsPerIPAddress.EnumString(), tenZeroZeroTwo)}, 0) - overriddenBucketKey, err := newIPAddressBucketKey(NewRegistrationsPerIPAddress, net.ParseIP(tenZeroZeroTwo)) test.AssertNotError(t, err, "should not error") overriddenLimit, err := txnBuilder.getLimit(NewRegistrationsPerIPAddress, overriddenBucketKey) @@ -87,12 +80,6 @@ func TestLimiter_CheckWithLimitOverrides(t *testing.T) { test.AssertEquals(t, d.remaining, int64(0)) test.AssertEquals(t, d.resetIn, time.Second) - // Verify our overrideUsageGauge is being set correctly. 1.0 == 100% - // of the bucket has been consumed. - test.AssertMetricWithLabelsEquals(t, l.overrideUsageGauge, prometheus.Labels{ - "limit_name": NewRegistrationsPerIPAddress.String(), - "bucket_key": joinWithColon(NewRegistrationsPerIPAddress.EnumString(), tenZeroZeroTwo)}, 1.0) - // Verify our RetryIn is correct. 1 second == 1000 milliseconds and // 1000/40 = 25 milliseconds per request. test.AssertEquals(t, d.retryIn, time.Millisecond*25) diff --git a/ratelimits/source_redis.go b/ratelimits/source_redis.go index 4d32f7c2a6d..ff32931efc2 100644 --- a/ratelimits/source_redis.go +++ b/ratelimits/source_redis.go @@ -99,10 +99,6 @@ func (r *RedisSource) BatchSet(ctx context.Context, buckets map[string]time.Time } totalLatency := r.clk.Since(start) - perSetLatency := totalLatency / time.Duration(len(buckets)) - for range buckets { - r.observeLatency("batchset_entry", perSetLatency, nil) - } r.observeLatency("batchset", totalLatency, nil) return nil @@ -128,17 +124,14 @@ func (r *RedisSource) BatchSetNotExisting(ctx context.Context, buckets map[strin alreadyExists := make(map[string]bool, len(buckets)) totalLatency := r.clk.Since(start) - perSetLatency := totalLatency / time.Duration(len(buckets)) for bucketKey, cmd := range cmds { success, err := cmd.Result() if err != nil { - r.observeLatency("batchsetnotexisting_entry", perSetLatency, err) return nil, err } if !success { alreadyExists[bucketKey] = true } - r.observeLatency("batchsetnotexisting_entry", perSetLatency, nil) } r.observeLatency("batchsetnotexisting", totalLatency, nil) @@ -163,11 +156,6 @@ func (r *RedisSource) BatchIncrement(ctx context.Context, buckets map[string]inc } totalLatency := r.clk.Since(start) - perSetLatency := totalLatency / time.Duration(len(buckets)) - for range buckets { - r.observeLatency("batchincrby_entry", perSetLatency, nil) - } - r.observeLatency("batchincrby", totalLatency, nil) return nil } @@ -211,7 +199,6 @@ func (r *RedisSource) BatchGet(ctx context.Context, bucketKeys []string) (map[st } totalLatency := r.clk.Since(start) - perEntryLatency := totalLatency / time.Duration(len(bucketKeys)) tats := make(map[string]time.Time, len(bucketKeys)) notFoundCount := 0 @@ -224,13 +211,10 @@ func (r *RedisSource) BatchGet(ctx context.Context, bucketKeys []string) (map[st r.observeLatency("batchget", r.clk.Since(start), err) return nil, err } - // Bucket key does not exist. - r.observeLatency("batchget_entry", perEntryLatency, err) notFoundCount++ continue } tats[bucketKeys[i]] = time.Unix(0, tatNano).UTC() - r.observeLatency("batchget_entry", perEntryLatency, nil) } var batchErr error diff --git a/ratelimits/transaction.go b/ratelimits/transaction.go index b5fd1653269..fa4b5e88715 100644 --- a/ratelimits/transaction.go +++ b/ratelimits/transaction.go @@ -403,7 +403,7 @@ func (builder *TransactionBuilder) certificatesPerDomainCheckOnlyTransactions(re return nil, err } if accountOverride { - if !perAccountLimit.isOverride() { + if !perAccountLimit.isOverride { return nil, fmt.Errorf("shouldn't happen: CertificatesPerDomainPerAccount limit is not an override") } perAccountPerDomainKey, err := NewRegIdDomainBucketKey(CertificatesPerDomainPerAccount, regId, name) @@ -481,7 +481,7 @@ func (builder *TransactionBuilder) CertificatesPerDomainSpendOnlyTransactions(re return nil, err } if accountOverride { - if !perAccountLimit.isOverride() { + if !perAccountLimit.isOverride { return nil, fmt.Errorf("shouldn't happen: CertificatesPerDomainPerAccount limit is not an override") } perAccountPerDomainKey, err := NewRegIdDomainBucketKey(CertificatesPerDomainPerAccount, regId, name) diff --git a/ratelimits/transaction_test.go b/ratelimits/transaction_test.go index 8cf0b798a1e..b4a25e837f0 100644 --- a/ratelimits/transaction_test.go +++ b/ratelimits/transaction_test.go @@ -79,14 +79,14 @@ func TestFailedAuthorizationsPerDomainPerAccountTransactions(t *testing.T) { test.AssertEquals(t, len(txns), 1) test.AssertEquals(t, txns[0].bucketKey, "4:123456789:so.many.labels.here.example.com") test.Assert(t, txns[0].checkOnly(), "should be check-only") - test.Assert(t, !txns[0].limit.isOverride(), "should not be an override") + test.Assert(t, !txns[0].limit.isOverride, "should not be an override") // A spend-only transaction for the default per-account limit. txn, err := tb.FailedAuthorizationsPerDomainPerAccountSpendOnlyTransaction(123456789, "so.many.labels.here.example.com") test.AssertNotError(t, err, "creating transaction") test.AssertEquals(t, txn.bucketKey, "4:123456789:so.many.labels.here.example.com") test.Assert(t, txn.spendOnly(), "should be spend-only") - test.Assert(t, !txn.limit.isOverride(), "should not be an override") + test.Assert(t, !txn.limit.isOverride, "should not be an override") // A check-only transaction for the per-account limit override. txns, err = tb.FailedAuthorizationsPerDomainPerAccountCheckOnlyTransactions(13371338, []string{"so.many.labels.here.example.com"}) @@ -94,14 +94,14 @@ func TestFailedAuthorizationsPerDomainPerAccountTransactions(t *testing.T) { test.AssertEquals(t, len(txns), 1) test.AssertEquals(t, txns[0].bucketKey, "4:13371338:so.many.labels.here.example.com") test.Assert(t, txns[0].checkOnly(), "should be check-only") - test.Assert(t, txns[0].limit.isOverride(), "should be an override") + test.Assert(t, txns[0].limit.isOverride, "should be an override") // A spend-only transaction for the per-account limit override. txn, err = tb.FailedAuthorizationsPerDomainPerAccountSpendOnlyTransaction(13371338, "so.many.labels.here.example.com") test.AssertNotError(t, err, "creating transaction") test.AssertEquals(t, txn.bucketKey, "4:13371338:so.many.labels.here.example.com") test.Assert(t, txn.spendOnly(), "should be spend-only") - test.Assert(t, txn.limit.isOverride(), "should be an override") + test.Assert(t, txn.limit.isOverride, "should be an override") } func TestFailedAuthorizationsForPausingPerDomainPerAccountTransactions(t *testing.T) { @@ -115,7 +115,7 @@ func TestFailedAuthorizationsForPausingPerDomainPerAccountTransactions(t *testin test.AssertNotError(t, err, "creating transaction") test.AssertEquals(t, txn.bucketKey, "8:13371338:so.many.labels.here.example.com") test.Assert(t, txn.check && txn.spend, "should be check and spend") - test.Assert(t, txn.limit.isOverride(), "should be an override") + test.Assert(t, txn.limit.isOverride, "should be an override") } func TestCertificatesPerDomainTransactions(t *testing.T) { @@ -153,7 +153,7 @@ func TestCertificatesPerDomainPerAccountTransactions(t *testing.T) { test.AssertEquals(t, len(txns), 1) test.AssertEquals(t, txns[0].bucketKey, "6:13371338:example.com") test.Assert(t, txns[0].checkOnly(), "should be check-only") - test.Assert(t, txns[0].limit.isOverride(), "should be an override") + test.Assert(t, txns[0].limit.isOverride, "should be an override") // Same as above, but with multiple example.com domains. txns, err = tb.certificatesPerDomainCheckOnlyTransactions(13371338, []string{"so.many.labels.here.example.com", "z.example.com"}) @@ -161,7 +161,7 @@ func TestCertificatesPerDomainPerAccountTransactions(t *testing.T) { test.AssertEquals(t, len(txns), 1) test.AssertEquals(t, txns[0].bucketKey, "6:13371338:example.com") test.Assert(t, txns[0].checkOnly(), "should be check-only") - test.Assert(t, txns[0].limit.isOverride(), "should be an override") + test.Assert(t, txns[0].limit.isOverride, "should be an override") // Same as above, but with different domains. txns, err = tb.certificatesPerDomainCheckOnlyTransactions(13371338, []string{"so.many.labels.here.example.com", "z.example.net"}) @@ -170,10 +170,10 @@ func TestCertificatesPerDomainPerAccountTransactions(t *testing.T) { test.AssertEquals(t, len(txns), 2) test.AssertEquals(t, txns[0].bucketKey, "6:13371338:example.com") test.Assert(t, txns[0].checkOnly(), "should be check-only") - test.Assert(t, txns[0].limit.isOverride(), "should be an override") + test.Assert(t, txns[0].limit.isOverride, "should be an override") test.AssertEquals(t, txns[1].bucketKey, "6:13371338:example.net") test.Assert(t, txns[1].checkOnly(), "should be check-only") - test.Assert(t, txns[1].limit.isOverride(), "should be an override") + test.Assert(t, txns[1].limit.isOverride, "should be an override") // Two spend-only transactions, one for the global limit and one for the // per-account limit override. @@ -183,11 +183,11 @@ func TestCertificatesPerDomainPerAccountTransactions(t *testing.T) { txns = sortTransactions(txns) test.AssertEquals(t, txns[0].bucketKey, "5:example.com") test.Assert(t, txns[0].spendOnly(), "should be spend-only") - test.Assert(t, !txns[0].limit.isOverride(), "should not be an override") + test.Assert(t, !txns[0].limit.isOverride, "should not be an override") test.AssertEquals(t, txns[1].bucketKey, "6:13371338:example.com") test.Assert(t, txns[1].spendOnly(), "should be spend-only") - test.Assert(t, txns[1].limit.isOverride(), "should be an override") + test.Assert(t, txns[1].limit.isOverride, "should be an override") } func TestCertificatesPerFQDNSetTransactions(t *testing.T) { @@ -202,7 +202,7 @@ func TestCertificatesPerFQDNSetTransactions(t *testing.T) { namesHash := fmt.Sprintf("%x", core.HashNames([]string{"example.com", "example.net", "example.org"})) test.AssertEquals(t, txn.bucketKey, "7:"+namesHash) test.Assert(t, txn.checkOnly(), "should be check-only") - test.Assert(t, !txn.limit.isOverride(), "should not be an override") + test.Assert(t, !txn.limit.isOverride, "should not be an override") } func TestNewTransactionBuilder(t *testing.T) {