refactor: create central configuration for caches#3026
Conversation
chronark
requested review from
MichaelUnkey,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
📝 WalkthroughWalkthroughThe changes add caching mechanisms across the application. New cache fields (e.g., Changes
Sequence Diagram(s)sequenceDiagram
participant H as Handler
participant C as Cache (SWR)
participant DB as Database
H->>C: Query for rate limit namespace/overrides
alt Cache Hit
C-->>H: Return cached data
else Cache Miss
H->>DB: Query rate limit data
DB-->>H: Return data or null
H->>C: Update cache with result
end
Suggested Reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (16)
go/apps/api/routes/v2_ratelimit_limit/400_test.go (1)
109-115: Consider updating the second test handler instanceThis test handler instance in
TestMissingAuthorizationHeaderis missing the newly added cache fields. For consistency, consider adding the same cache dependencies here as well.route := handler.New(handler.Services{ - DB: h.DB, - Keys: h.Keys, - Logger: h.Logger, - Permissions: h.Permissions, - Ratelimit: h.Ratelimit, + DB: h.DB, + Keys: h.Keys, + Logger: h.Logger, + Permissions: h.Permissions, + Ratelimit: h.Ratelimit, + RatelimitNamespaceByNameCache: h.Caches.RatelimitNamespaceByName, + RatelimitOverrideMatchesCache: h.Caches.RatelimitOverridesMatch, })go/pkg/otel/metrics/metrics.go (1)
157-157: Small example usage inconsistency.There's a minor typo in the example usage - it shows
metrics.Caches.Revalidationsbut the struct field is actuallymetrics.Cache.Revalidations.-// metrics.Caches.Revalidations.Add(ctx, 1, metric.WithAttributes( +// metrics.Cache.Revalidations.Add(ctx, 1, metric.WithAttributes(go/pkg/testutil/http.go (2)
38-38: Add a clarifying docstring for the Caches field
A short comment explaining the role ofCachesin the test harness may improve maintainability.
62-66: Initialize real caches in the test harness with caution
Using complete cache instances in tests can expose real-world issues but may slow down test execution. Consider using minimal time windows or mocking for faster unit tests.go/internal/services/caches/caches.go (1)
33-40: Extend cache configuration flexibility
If you anticipate varying requirements (e.g., different TTLs or resource constraints), consider allowing custom overrides inConfigto avoid code changes for each environment.go/pkg/cache/cache.go (5)
20-22: Clarify field purpose or add doc comments.
These fields (otter,fresh,stale) are central to cache behavior but lack descriptive documentation. Adding short comments describing their roles would improve maintainability.
62-68: Use domain-specific cost function if applicable.
The cost function for.Cost(...)is set to a constant 1. Ensure this simplistic approach aligns with your usage patterns. If certain entries have different complexities or memory footprints, consider tailoring the cost function accordingly.
85-91: Avoid magic numbers for goroutine count.
Spawning exactly 10 goroutines in a loop is a “magic number.” Consider making this configurable or referencing a named constant to clarify its intent and allow easy future adjustments.
252-252: Consider making revalidation public only if needed.
Therevalidatemethod is private; if external usage is anticipated, consider adjusting its visibility. Otherwise, keep it unexported to limit the API surface.
275-277: Log critical details on revalidation errors.
Thec.logger.Warn("failed to revalidate", …)call is helpful, but including the cache resource name or other context attributes may assist in debugging.go/apps/api/run.go (3)
105-111: Check for cache creation errors early.
You correctly checkerrafter callingcaches.New(...). If this is critical for the API to function, consider providing fallback or additional logging for debug.
169-169: Permission cache usage.
Referencingcaches.PermissionsByKeyIdin thepermissionsservice is beneficial for performance. Ensure usage patterns (e.g., stale intervals) match typical user loads.
183-183: Expose caches in routes carefully.
AddingCaches: cachestoroutes.Servicesis fine if multiple routes rely on them. Consider limiting direct usage if only a few routes need these caches to avoid tight coupling across your modules.go/apps/api/routes/v2_ratelimit_limit/handler.go (3)
27-33: Eliminate potential code duplication.
Defining new fields forRatelimitNamespaceByNameCacheandRatelimitOverrideMatchesCacheis good, but watch for patterns in other packages. If repeated often, consider a more generic approach to keep code DRY.
72-75: WriteNull for no rows.
Temporarily storing a null entry prevents unnecessary repeated queries. Confirm that an alternative flow is available if the namespace is created after returning no rows.
129-132: Check override results carefully.
Storing multiple overrides in the cache can be beneficial, but ensure partial results (e.g., a single override) don't overshadow others.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
go/apps/api/integration/multi_node_ratelimiting/accuracy_test.go(1 hunks)go/apps/api/routes/register.go(1 hunks)go/apps/api/routes/services.go(2 hunks)go/apps/api/routes/v2_ratelimit_limit/200_test.go(1 hunks)go/apps/api/routes/v2_ratelimit_limit/400_test.go(1 hunks)go/apps/api/routes/v2_ratelimit_limit/401_test.go(1 hunks)go/apps/api/routes/v2_ratelimit_limit/403_test.go(1 hunks)go/apps/api/routes/v2_ratelimit_limit/404_test.go(1 hunks)go/apps/api/routes/v2_ratelimit_limit/accuracy_test.go(1 hunks)go/apps/api/routes/v2_ratelimit_limit/handler.go(4 hunks)go/apps/api/run.go(5 hunks)go/internal/services/caches/caches.go(1 hunks)go/internal/services/caches/doc.go(1 hunks)go/internal/services/keys/service.go(2 hunks)go/internal/services/permissions/service.go(1 hunks)go/pkg/cache/cache.go(7 hunks)go/pkg/error_code/codes.go(0 hunks)go/pkg/error_code/unkey_database_not_unique.go(0 hunks)go/pkg/error_code/unkey_database_transaction_timeout.go(0 hunks)go/pkg/error_code/unkey_key_not_found.go(0 hunks)go/pkg/otel/metrics/metrics.go(2 hunks)go/pkg/testutil/http.go(5 hunks)
💤 Files with no reviewable changes (4)
- go/pkg/error_code/unkey_database_not_unique.go
- go/pkg/error_code/unkey_database_transaction_timeout.go
- go/pkg/error_code/unkey_key_not_found.go
- go/pkg/error_code/codes.go
🧰 Additional context used
🧬 Code Definitions (11)
go/apps/api/routes/v2_ratelimit_limit/404_test.go (2)
go/pkg/testutil/http.go (3)
h(133-142)h(145-147)h(149-151)go/internal/services/caches/caches.go (1)
Caches(15-31)
go/apps/api/routes/v2_ratelimit_limit/200_test.go (2)
go/pkg/testutil/http.go (3)
h(133-142)h(145-147)h(149-151)go/internal/services/caches/caches.go (1)
Caches(15-31)
go/apps/api/routes/services.go (1)
go/internal/services/caches/caches.go (1)
Caches(15-31)
go/apps/api/routes/v2_ratelimit_limit/401_test.go (2)
go/pkg/testutil/http.go (3)
h(133-142)h(145-147)h(149-151)go/internal/services/caches/caches.go (1)
Caches(15-31)
go/apps/api/routes/v2_ratelimit_limit/accuracy_test.go (2)
go/pkg/testutil/http.go (3)
h(133-142)h(145-147)h(149-151)go/internal/services/caches/caches.go (1)
Caches(15-31)
go/internal/services/permissions/service.go (2)
go/pkg/cache/interface.go (1)
Cache(7-31)go/pkg/cache/cache.go (2)
Config(33-50)cache(19-31)
go/pkg/testutil/http.go (1)
go/internal/services/caches/caches.go (3)
Caches(15-31)New(71-130)Config(34-40)
go/apps/api/routes/v2_ratelimit_limit/handler.go (3)
go/internal/services/ratelimit/interface.go (1)
Service(10-12)go/pkg/cache/interface.go (2)
Cache(7-31)Op(50-50)go/pkg/cache/cache.go (1)
cache(19-31)
go/apps/api/run.go (5)
go/internal/services/keys/service.go (2)
New(24-31)Config(10-15)go/internal/services/permissions/service.go (2)
New(29-37)Config(22-27)go/internal/services/caches/caches.go (3)
New(71-130)Config(34-40)Caches(15-31)go/internal/services/ratelimit/service.go (2)
New(59-86)Config(53-57)go/apps/api/config.go (1)
Config(8-74)
go/apps/api/routes/v2_ratelimit_limit/403_test.go (2)
go/pkg/testutil/http.go (3)
h(133-142)h(145-147)h(149-151)go/internal/services/caches/caches.go (1)
Caches(15-31)
go/pkg/cache/cache.go (4)
go/pkg/otel/metrics/metrics.go (2)
Cache(43-161)err(205-205)go/pkg/cache/interface.go (2)
Cache(7-31)Op(50-50)go/pkg/cache/entry.go (1)
swrEntry(7-16)go/pkg/otel/logging/interface.go (1)
Logger(11-116)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Test Go API Local / Test (Shard 1/8)
- GitHub Check: Test Go API Local / Test (Shard 2/8)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Build / Build
🔇 Additional comments (33)
go/apps/api/routes/services.go (1)
4-4: Good centralization of cache mechanisms.The addition of the
cachesimport and theCachesfield to theServicesstruct implements a clean centralization of caching capabilities. This approach makes the caching system more modular and maintainable.Also applies to: 26-26
go/apps/api/routes/v2_ratelimit_limit/404_test.go (1)
28-29: Appropriate integration of cache services in the test.The test now correctly utilizes the centralized caching mechanism. These additions align with the overall refactoring to use a centralized cache configuration across the application.
go/apps/api/routes/v2_ratelimit_limit/403_test.go (1)
39-40: Consistent implementation of caching in tests.The test properly integrates the centralized caching mechanism, maintaining consistency with other test files in the codebase.
go/apps/api/routes/v2_ratelimit_limit/accuracy_test.go (1)
66-67: Efficient use of caching in performance-sensitive tests.The integration of caching in this accuracy test is particularly valuable, as rate limit accuracy tests involve multiple requests that can benefit from cached lookups, potentially improving both test performance and reliability.
go/internal/services/caches/doc.go (1)
1-16: Well-structured package documentationThe package documentation for
cachesclearly explains its purpose, use cases, and implementation details. It follows Go's documentation standards and provides valuable context for developers.go/apps/api/routes/v2_ratelimit_limit/400_test.go (1)
22-29: Proper integration of cache dependenciesThe addition of
RatelimitNamespaceByNameCacheandRatelimitOverrideMatchesCacheto the services struct ensures that the test properly utilizes the centralized caching mechanism, which should improve performance by reducing database load.go/apps/api/routes/v2_ratelimit_limit/401_test.go (1)
15-23: Proper integration of cache dependenciesThe addition of caching fields
RatelimitNamespaceByNameCacheandRatelimitOverrideMatchesCachefollows the same pattern as other test files, maintaining consistency in how services are initialized across tests.go/apps/api/routes/v2_ratelimit_limit/200_test.go (1)
32-40: Proper integration of cache dependenciesThe addition of
RatelimitNamespaceByNameCacheandRatelimitOverrideMatchesCacheto the handler services is consistent with the changes in other test files. This ensures that the test utilizes the centralized caching mechanism for better performance.When testing with rate limit overrides (like in the "with override" test case on line 92-126), these cache dependencies should help reduce database load by caching the namespace and override lookups.
go/apps/api/routes/register.go (1)
47-53: LGTM! Properly integrated caching services.The addition of the two new cache fields to the
v2RatelimitLimit.Servicesstruct initialization looks good. These new cache fields appropriately reference the centralized cache instances fromsvc.Caches. This change is consistent with the PR objective of creating a central configuration for caches.go/pkg/otel/metrics/metrics.go (2)
150-160: Good addition of Revalidations metric for cache monitoring.The new
Revalidationsmetric is a valuable addition for monitoring cache performance and will help track how frequently caches undergo background revalidation.
270-273: LGTM! Good metric initialization.The initialization of the new
Revalidationscounter metric follows the same pattern used for other metrics in the file, with proper error handling.go/apps/api/integration/multi_node_ratelimiting/accuracy_test.go (2)
125-125: Improved logging clarity.Adding the number of windows to the log output provides better context for understanding the test results and makes debugging easier.
128-130: Improved error message consistency.Changing from "Success count" to "Passed count" in the error messages makes the terminology more consistent with the variable names used in the test, improving readability.
go/internal/services/permissions/service.go (2)
26-26: Good refactoring to centralize cache configuration.Adding the
Cachefield to theConfigstruct is a good approach for centralized cache configuration. This allows the cache to be created and configured externally and injected into the service.
35-35: Clean cache injection implementation.Removing the inline cache creation and directly using the injected cache simplifies the code and aligns with the central configuration objective. This approach makes cache configuration more maintainable across the application.
go/internal/services/keys/service.go (2)
11-14: Add nil-check for KeyCache to avoid potential runtime errors
IfKeyCacheis not provided in the config, this field could be nil and might cause panics at runtime. Consider either documenting the requirement for a non-nil cache or adding a fallback or error check upon initialization.Would you like me to generate a script to scan usage of
KeyCacheacross the codebase to confirm no nil dereferences?
29-29: Assigning injected KeyCache looks correct
Leveragingconfig.KeyCachedirectly in the service constructor aligns with the new caching strategy and ensures consistency of configuration between tests and production.go/pkg/testutil/http.go (4)
12-12: New import for caching utility
Importing thecachespackage in the test harness centralizes cache management for test scenarios.
75-78: Injecting KeyByHash cache
This is consistent with the external caching approach. Ensure all references tokeys.Newacross the test suite now rely on this injection and do not create any redundant caches.
89-89: PermissionsByKeyId cache usage
Injecting the permissions cache follows the same central caching strategy. No issues here.
118-118: Expose caches in Harness
Exposing the newly created caches allows for shared usage in tests, improving maintainability and consistency. Good approach.go/internal/services/caches/caches.go (2)
1-31: Centralized caching struct
Defining specialized fields for core domain entities fosters clarity and maintainability. Overall design looks solid.
42-130: Thorough cache initialization
The structured approach with well-defined sizes, freshness/staleness windows, and tracing middleware is commendable. Confirm these defaults meet production needs.go/pkg/cache/cache.go (3)
80-80: Validate concurrency strategy.
Creating the revalidation channel with a buffer size of 1000 is a good start, but confirm that potential usage won't exceed this limit under peak load.
240-240: Confirm restoration logic for stale entries.
The conditionif now.Before(entry.Fresh) || now.Before(entry.Stale)might restore entries that are already in the stale threshold. Verify that indeed aligns with your cache semantics (some may prefer requiring both conditions to hold).
273-273: Track revalidation rates carefully.
While incrementingmetrics.Cache.Revalidationsis useful, confirm you have alerting or dashboards to watch for unexpected surges in revalidation.go/apps/api/run.go (2)
14-14: Validate import usage.
The newcachesimport helps unify caching logic. Ensure no cyclic dependencies arise and that grouping is consistent with existing import patterns.
128-132: Good injection of KeyCache.
Passingcaches.KeyByHashintokeys.New(...)encourages better testability and decoupling. Make sure to test cache hits/misses thoroughly.go/apps/api/routes/v2_ratelimit_limit/handler.go (5)
5-6: Check error import ordering.
Adding"database/sql"and"errors"is correct for handling null/no-row scenarios. Ensure consistent error handling for all query logic.
14-14: Cache interface usage.
Importing yourcachepackage is consistent with the new SWR approach. Confirm a well-tested path for queries returning no rows.
62-64: Validate query arguments.
ConstructingfindNamespaceArgsis straightforward, but consider adding logs or checks ifWorkspaceIDorNamemight be uninitialized.
65-68: SWR approach is solid.
Calling.SWR(...)to handle stale data and revalidate fromdb.Query.FindRatelimitNamespaceByNameis a clean solution. Keep an eye on cache revalidation frequency to avoid overhead.
123-127: Reuse argument struct for override queries.
ThefindOverrideMatchesArgsstruct is a neat parallel to the namespace approach. This consistency helps maintain the code.
| resource string | ||
| clock clock.Clock | ||
|
|
||
| revalidateC chan func() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure channel capacity is sufficient.
A fixed buffered channel (revalidateC) of size 1000 may risk blocking if revalidation spikes occur. Consider making its size configurable or using a non-blocking approach to prevent potential backpressure.
| c.revalidateC <- func() { | ||
| c.revalidate(ctx, key, refreshFromOrigin, op) | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent channel overflow.
Pushing tasks to revalidateC in the SWR path is fine, but consider how to handle scenarios where the queue is full or revalidation tasks arrive faster than they can be processed.
2 participants
Summary by CodeRabbit
New Features
Refactor
Chore