Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ratelimits: Fix cardinality explosion in overrideUsageGauge #7548

Merged
merged 1 commit into from
Jun 14, 2024
Merged

ratelimits: Fix cardinality explosion in overrideUsageGauge #7548

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ratelimits/bucket.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,7 @@ func (builder *TransactionBuilder) CertificatesPerDomainTransactions(regId int64
if err != nil {
return nil, err
}
if perAccountLimit.isOverride {
if perAccountLimit.isOverride() {
// An override is configured for the CertificatesPerDomainPerAccount
// limit.
perAccountPerDomainKey, err := newRegIdDomainBucketKey(CertificatesPerDomainPerAccount, regId, name)
Expand Down
10 changes: 4 additions & 6 deletions ratelimits/gcra_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,8 @@ import (

func TestDecide(t *testing.T) {
clk := clock.NewFake()
limit := precomputeLimit(
limit{Burst: 10, Count: 1, Period: config.Duration{Duration: time.Second}},
)
limit := limit{Burst: 10, Count: 1, Period: config.Duration{Duration: time.Second}}
limit.precompute()

// Begin by using 1 of our 10 requests.
d := maybeSpend(clk, limit, clk.Now(), 1)
Expand Down Expand Up @@ -135,9 +134,8 @@ func TestDecide(t *testing.T) {

func TestMaybeRefund(t *testing.T) {
clk := clock.NewFake()
limit := precomputeLimit(
limit{Burst: 10, Count: 1, Period: config.Duration{Duration: time.Second}},
)
limit := limit{Burst: 10, Count: 1, Period: config.Duration{Duration: time.Second}}
limit.precompute()

// Begin by using 1 of our 10 requests.
d := maybeSpend(clk, limit, clk.Now(), 1)
Expand Down
24 changes: 16 additions & 8 deletions ratelimits/limit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,15 +44,19 @@ type limit struct {
// precomputed to avoid doing the same calculation on every request.
burstOffset int64

// isOverride is true if this limit is an override limit, false if it is a
// default limit.
isOverride bool
// overrideKey is the key used to look up this limit in the overrides map.
overrideKey string
}

func precomputeLimit(l limit) limit {
// isOverride returns true if the limit is an override.
func (l *limit) isOverride() bool {
return l.overrideKey != ""
}

// precompute calculates the emissionInterval and burstOffset for the limit.
func (l *limit) precompute() {
l.emissionInterval = l.Period.Nanoseconds() / l.Count
l.burstOffset = l.emissionInterval * l.Burst
return l
}

func validateLimit(l limit) error {
Expand Down Expand Up @@ -157,21 +161,24 @@ func loadAndParseOverrideLimits(path string) (limits, error) {
return nil, fmt.Errorf("unrecognized name %q in override limit, must be one of %v", k, limitNames)
}
v.limit.name = name
v.limit.isOverride = true

for _, entry := range v.Ids {
limit := v.limit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since v.limit doesn't vary from loop to loop, it would make more sense to do this assignment outside the loop. If you move it all the way to the top of the containing loop (L155), you can also replace some other instances of v.limit with just limit (which I believe is the purpose of the assignment, to create a shorthand).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of this assignment is to create a copy-by-value of the limit, so that we can set fields which do vary per-loop (namely the overrideKey) without affecting the v.limit object from the parent loop.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was one of those things that didn't need to be fixed but could certainly lead to some unpleasantness if someone were to refactor it at a later date. So we decided to fix it now.

id := entry.Id
err = validateIdForName(name, id)
if err != nil {
return nil, fmt.Errorf(
"validating name %s and id %q for override limit %q: %w", name, id, k, err)
}
limit.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, ",")))
}
parsed[joinWithColon(name.EnumString(), id)] = precomputeLimit(v.limit)
limit.precompute()
parsed[joinWithColon(name.EnumString(), id)] = limit
}
}
}
Expand All @@ -197,7 +204,8 @@ func loadAndParseDefaultLimits(path string) (limits, error) {
return nil, fmt.Errorf("unrecognized name %q in default limit, must be one of %v", k, limitNames)
}
v.name = name
parsed[name.EnumString()] = precomputeLimit(v)
v.precompute()
parsed[name.EnumString()] = v
}
return parsed, nil
}
Expand Down
4 changes: 2 additions & 2 deletions ratelimits/limiter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,9 +196,9 @@ func (l *Limiter) BatchSpend(ctx context.Context, txns []Transaction) (*Decision

d := maybeSpend(l.clk, txn.limit, tat, txn.cost)

if txn.limit.isOverride {
if txn.limit.isOverride() {
utilization := float64(txn.limit.Burst-d.Remaining) / float64(txn.limit.Burst)
l.overrideUsageGauge.WithLabelValues(txn.limit.name.String(), txn.bucketKey).Set(utilization)
l.overrideUsageGauge.WithLabelValues(txn.limit.name.String(), txn.limit.overrideKey).Set(utilization)
}

if d.Allowed && (tat != d.newTAT) && txn.spend {
Expand Down