Skip to content

chore: add metrics and tests to all cache implementations#2874

Merged
miparnisari merged 1 commit intomainfrom
add-metrics-to-caches
Feb 4, 2026
Merged

chore: add metrics and tests to all cache implementations#2874
miparnisari merged 1 commit intomainfrom
add-metrics-to-caches

Conversation

@miparnisari
Copy link
Contributor

@miparnisari miparnisari commented Feb 3, 2026

No description provided.

@github-actions github-actions bot added the area/cli Affects the command line label Feb 3, 2026
@codecov
Copy link

codecov bot commented Feb 3, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 65.07937% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.88%. Comparing base (c2c3bab) to head (0393250).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/cmd/server/cacheconfig.go 0.00% 12 Missing ⚠️
pkg/cache/cache_theine.go 66.67% 5 Missing and 1 partial ⚠️
pkg/cache/cache_otter.go 86.37% 2 Missing and 1 partial ⚠️
pkg/cache/cache.go 0.00% 1 Missing ⚠️

❌ Your project status has failed because the head coverage (74.88%) is below the target coverage (75.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files 
@@             Coverage Diff             @@
##             main    #2874       +/-   ##
===========================================
+ Coverage   50.02%   74.88%   +24.87%     
===========================================
  Files         416      484       +68     
  Lines       51735    57869     +6134     
===========================================
+ Hits        25873    43328    +17455     
+ Misses      23288    11506    -11782     
- Partials     2574     3035      +461

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@miparnisari miparnisari force-pushed the add-metrics-to-caches branch 2 times, most recently from f0c44c8 to 07633a8 Compare February 4, 2026 20:06
@miparnisari miparnisari changed the title chore: add metrics to cache implementations to try them out chore: add metrics and tests to all cache implementations Feb 4, 2026
@github-actions github-actions bot added the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Feb 4, 2026
miparnisari
miparnisari commented Feb 4, 2026
View reviewed changes
pkg/cache/cache_ristretto.go
Copy link
Contributor Author

@miparnisari miparnisari Feb 4, 2026

Choose a reason for hiding this comment

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

FYI this is the one used in prod today

@miparnisari miparnisari marked this pull request as ready for review February 4, 2026 20:08
@miparnisari miparnisari requested a review from a team as a code owner February 4, 2026 20:08
tstirrat15
tstirrat15 approved these changes Feb 4, 2026
View reviewed changes
Copy link
Contributor

@tstirrat15 tstirrat15 left a comment

Choose a reason for hiding this comment

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

LGTM, but see question about otter

pkg/cache/cache_ristretto.go
}

cache := wrapped[K, V]{name, config, config.DefaultTTL, rcache}
cache := ristretoCache[K, V]{name, config, config.DefaultTTL, rcache}
Copy link
Contributor

@tstirrat15 tstirrat15 Feb 4, 2026

Choose a reason for hiding this comment

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

Suggested change
cache := ristretoCache[K, V]{name, config, config.DefaultTTL, rcache}
cache := ristrettoCache[K, V]{name, config, config.DefaultTTL, rcache}

Plus other updates

Copy link
Contributor Author

@miparnisari miparnisari Feb 4, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

follow-up PR :D

pkg/cache/cache_otter.go
}

wtc.metrics.costAdded.Add(uint64(uintCost))
_, ok := wtc.Get(key)
Copy link
Contributor

@tstirrat15 tstirrat15 Feb 4, 2026

Choose a reason for hiding this comment

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

I wonder if there's a meaningful difference between Otter and Ristretto such that this bookkeeping isn't required?

Copy link
Contributor Author

@miparnisari miparnisari Feb 4, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

More generally i think the semantics of this entire Cache interface are unclear. What is Set supposed to return if the key exists?

Theine overwrites.
Otter fails if it already exists.
Ristretto overwrites, i think.

I don't know how the callers expect Set to behave to say how each impl should behave.

@miparnisari miparnisari enabled auto-merge (squash) February 4, 2026 22:21
@miparnisari miparnisari force-pushed the add-metrics-to-caches branch from 07633a8 to c02161f Compare February 4, 2026 22:34
@miparnisari miparnisari force-pushed the add-metrics-to-caches branch from c02161f to 0393250 Compare February 4, 2026 23:03
@miparnisari miparnisari merged commit d753f26 into main Feb 4, 2026
44 of 45 checks passed
@miparnisari miparnisari deleted the add-metrics-to-caches branch February 4, 2026 23:15
@github-actions github-actions bot locked and limited conversation to collaborators Feb 4, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@tstirrat15 tstirrat15 tstirrat15 approved these changes

Assignees

No one assigned

Labels

area/cli Affects the command line area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@miparnisari @tstirrat15