chore: use less locking in metrics hot paths - #213
Merged
sighphyre merged 3 commits intoNov 21, 2025
Merged
Conversation
sighphyre
changed the base branch from
v5
to
fix/use-sync-pool-for-strategy-randomness
November 19, 2025 13:58
Pull Request Test Coverage Report for Build 19563538142Details
💛 - Coveralls |
sighphyre
commented
Nov 19, 2025
| return actual.(*toggleCounters) | ||
| } | ||
|
|
||
| func (m *metrics) reinsertBucket(bucket api.Bucket) { |
Member
Author
There was a problem hiding this comment.
Previously the code just dropped variants on the floor when re-inserting. Needed to get fixed and now seems like a good time I guess so here's a fix
sighphyre
commented
Nov 19, 2025
| atomic.AddInt64(&c.yes, int64(num)) | ||
| } else { | ||
| t.No += num | ||
| atomic.AddInt64(&c.no, int64(num)) |
Member
Author
There was a problem hiding this comment.
Wheee atomic primitives are awesome. We can do this on the hardware level and not sandwiched between the runtime and OS
sighphyre
commented
Nov 19, 2025
| st.Expect(t, gock.IsDone(), true) | ||
| } | ||
|
|
||
| func TestReinsertingBucketsAlsoRestoresVariants(t *testing.T) { |
Member
Author
There was a problem hiding this comment.
New test for the re-insert variant logic
sighphyre
changed the title
sighphyre
marked this pull request as ready for review
November 19, 2025 14:07
sighphyre
force-pushed
the
fix/use-sync-pool-for-strategy-randomness
branch
from
November 20, 2025 12:49
db94232 to
e39f515
Compare
sighphyre
force-pushed
the
chore/rework-metrics-to-be-less-locky
branch
from
November 20, 2025 12:56
2cf51a1 to
bd4c12b
Compare
sighphyre
force-pushed
the
fix/use-sync-pool-for-strategy-randomness
branch
from
November 20, 2025 13:21
e39f515 to
1c38a81
Compare
sighphyre
force-pushed
the
chore/rework-metrics-to-be-less-locky
branch
from
November 20, 2025 13:27
bd4c12b to
2b95c7f
Compare
FredrikOseberg
approved these changes
Nov 20, 2025
FredrikOseberg
left a comment
FredrikOseberg
left a comment
Contributor
There was a problem hiding this comment.
Some nits, otherwise LGTM
| } | ||
| } | ||
|
|
||
| tc := api.ToggleCount{ |
Contributor
There was a problem hiding this comment.
toggleCount would be clearer
Member
Author
There was a problem hiding this comment.
Yep, took that suggestion. Uh... is there a pattern for this? The default Go community stance seems to be make variables as short and difficult to read as possible so I've accidentally followed that sometimes but I hate it. Is the a rule where we shouldn't do that?
|
|
||
| m.counters.Range(func(key, value any) bool { | ||
| name := key.(string) | ||
| c := value.(*toggleCounters) |
Contributor
There was a problem hiding this comment.
counters? I find myself scrolling up to figure out what this variable is.
|
|
||
| func (m *metrics) reinsertBucket(bucket api.Bucket) { | ||
| for name, tc := range bucket.Toggles { | ||
| c := m.getOrCreateCounter(name) |
Member
Author
There was a problem hiding this comment.
Also fair, also patched
github-project-automation
Bot
moved this from In Progress
to Approved PRs
in Issues and PRs
sighphyre
force-pushed
the
chore/rework-metrics-to-be-less-locky
branch
from
November 20, 2025 15:06
4adee3f to
19b67c5
Compare
* fix: variant strategies now correctly inherit stickiness from their strategy * chore: make metric count channels best effort send (#214)
sighphyre
merged commit Nov 21, 2025
09b1a34
into
fix/use-sync-pool-for-strategy-randomness
5 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Yeets the locking in the metrics code in favor of sync primitives. Fixes a bug in the re-insertion code since I was in that space. Variants are still behind a mutex but that mutex is now smaller scoped and locked less often. We can probably remove that too but its a bigger PR and I want to keep this focused on the hot path