Increase orderbook mid price cache duration to 60 seconds #2633
Conversation
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
indexer/packages/redis/__tests__/caches/orderbook-mid-prices-cache.test.ts (2)
139-139: Verify test coverage for edge cases with new cache durationThe test has been updated to reflect the new 60s cache duration, but consider adding test cases for:
- Exactly 60s (boundary condition)
- 59s (just before expiry)
- Multiple cache cycles to ensure consistent behavior
Here's a suggested addition to improve test coverage:
it('returns the correct median price after 60 seconds', async () => { // ... existing test code ... }); + +it('handles boundary conditions for cache expiry', async () => { + jest.useFakeTimers(); + const mockPrices = ['50000', '51000']; + + (OrderbookLevelsCache.getOrderBookMidPrice as jest.Mock) + .mockResolvedValueOnce(mockPrices[0]) + .mockResolvedValueOnce(mockPrices[1]); + + await fetchAndCacheOrderbookMidPrices(client, [defaultTicker]); + + // Test at exactly 60s + jest.advanceTimersByTime(60000); + let result = await getMedianPrices(client, [defaultTicker]); + expect(result[defaultTicker]).toBeDefined(); + + // Test just before expiry + jest.advanceTimersByTime(59000); + result = await getMedianPrices(client, [defaultTicker]); + expect(result[defaultTicker]).toBeDefined(); + + jest.useRealTimers(); +});Also applies to: 157-157
157-157: Consider adjusting timer advancement for better clarityThe current test advances time by 61 seconds. While this works, using exactly 60 seconds would be more intuitive and align better with the actual cache duration. The extra second isn't necessary since Redis's ZREMRANGEBYSCORE is inclusive.
-jest.advanceTimersByTime(61000); // Advance time by 61 seconds +jest.advanceTimersByTime(60000); // Advance time by exactly 60 seconds
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
indexer/packages/redis/__tests__/caches/orderbook-mid-prices-cache.test.ts(2 hunks)indexer/packages/redis/src/scripts/add_orderbook_mid_prices.lua(2 hunks)
🔇 Additional comments (2)
indexer/packages/redis/src/scripts/add_orderbook_mid_prices.lua (2)
10-11: LGTM: Variable name change reflects new cache duration
The variable name change from thirtySeconds to sixtySeconds accurately reflects the new cache duration.
19-19: Consider memory usage impact with increased cache duration
Doubling the cache duration from 30s to 60s means Redis will store twice as much price data per market. While this helps smooth out price spikes, it's important to monitor Redis memory usage, especially for markets with high update frequencies.
Let's analyze the potential memory impact:
Also applies to: 34-34
✅ Verification successful
Based on the analysis of the code and configuration, I can now generate the final response:
Memory impact of increased cache duration is minimal and controlled
The Lua script implementation shows that Redis automatically removes old entries using ZREMRANGEBYSCORE, maintaining a fixed time window of data regardless of the duration. The change from 30s to 60s will have a linear and predictable impact:
- Each market's price data is stored in a sorted set with timestamp-based cleanup
- Old entries beyond the time window are automatically pruned
- Memory usage scales linearly with update frequency × number of markets × time window
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check current memory usage pattern for price cache keys
redis-cli --eval "
local keys = redis.call('KEYS', 'orderbook-mid-prices:*')
local total = 0
local stats = {}
for _, key in ipairs(keys) do
local size = redis.call('ZCARD', key)
total = total + size
stats[key] = size
end
return {total, stats}
"
Length of output: 376
Script:
#!/bin/bash
# Let's analyze the code to understand the memory impact
# First, let's find all references to this cache duration
rg "sixtySeconds" -A 3 -B 3
# Then check for any configuration or constants related to this duration
rg -i "cache.*duration|ttl" -A 2 -B 2
# Look for any Redis-related configuration files
fd -e yaml -e json -e toml -e conf . | xargs rg -i "redis"
# Check the Lua script usage pattern
rg "add_orderbook_mid_prices.lua" -A 3 -B 3
Length of output: 76502
Script:
#!/bin/bash
# Let's analyze the Redis usage pattern and memory implications
# First, check the Lua script implementation
cat indexer/packages/redis/src/scripts/add_orderbook_mid_prices.lua
# Look for any Redis configuration files or settings
fd -e ts -e js -e json . | xargs rg -i "redis.*config|redis.*option"
# Check for any memory-related configurations or settings
fd -e ts -e js -e json . | xargs rg -i "redis.*memory|cache.*size|cache.*limit"
# Look for any orderbook or price-related configurations
fd -e ts -e js -e json . | xargs rg -i "orderbook.*config|price.*config"
Length of output: 13163
|
@mergify backport release/indexer/v7x |
❌ No backport have been created
GitHub error: |
|
@mergify backport release/indexer/v8.x |
✅ Backports have been created
|
|
@mergify backport release/indexer/v7.x |
✅ Backports have been created
|
(cherry picked from commit 7da33a6)
(cherry picked from commit 7da33a6)
Increases the amount of time we store orderbook mid price data from 30 to 60 seconds. This is to remedy spikes in candle data that we continue to see with 30 seconds of data.
Summary by CodeRabbit
Bug Fixes
getMedianPricetest case from 30 seconds to 60 seconds to ensure accurate median price verification.Chores