Skip to content

Commit

Permalink
fix: no need to store decisions in a list in redis (#1151)
Browse files Browse the repository at this point in the history
## Which problem is this PR solving?

A trace state is stored in two data structures in redis currently, one
in a hash table with the rest of the `CentralTraceStatus` information,
another one in a sorted set. The sorted set is used for
`GetTracesForState` so that Refinery can quickly get all trace IDs
within a state. Refinery never retrieves a list of trace IDs that has
their sampling decisions already made. Therefore, we actually don't need
to store than in a sorted set form anymore

This should further reduce our Redis memory usage.

## Short description of the changes

- don't store trace IDs in a state if it's decision is already made

---------

Co-authored-by: Kent Quirk <kentquirk@honeycomb.io>
  • Loading branch information
VinozzZ and kentquirk authored May 20, 2024
1 parent a0c62d7 commit 02738c8
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 2 deletions.
2 changes: 1 addition & 1 deletion .tool-versions
Original file line number Diff line number Diff line change
@@ -1 +1 @@
golang 1.22
golang 1.22.3
11 changes: 10 additions & 1 deletion centralstore/redis_store.go
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,9 @@ func (r *RedisBasicStore) RecordMetrics(ctx context.Context) error {
defer unlock()

for _, state := range r.states.states {
if state == DecisionDrop || state == DecisionKeep {
continue
}
// get the state counts
count, err := conn.ZCount(r.states.stateNameKey(state), 0, -1)
if err != nil {
Expand Down Expand Up @@ -1163,6 +1166,9 @@ func (t *traceStateProcessor) removeExpiredTraces(ctx context.Context, client re

// get the traceIDs that have been in the state for longer than the expiration time
for _, state := range t.states {
if state == DecisionKeep || state == DecisionDrop {
continue
}
replies, err := t.config.removeExpiredTraces.DoInt(ctx, conn, t.stateNameKey(state),
t.clock.Now().Add(-t.config.maxTraceRetention).UnixMicro(),
t.config.reaperBatchSize)
Expand Down Expand Up @@ -1321,7 +1327,10 @@ const traceStateChangeScript = `
-- the construction of the key for the sorted set should match with the stateNameKey function
-- in the traceStateProcessor struct
local added = redis.call('ZADD', string.format("%s:traces", nextState), "NX", timestamp, traceID)
if (nextState ~= "decision_keep") and (nextState ~= "decision_drop") then
local added = redis.call('ZADD', string.format("%s:traces", nextState), "NX", timestamp, traceID)
end
local removed = redis.call('ZREM', string.format("%s:traces", currentState), traceID)
Expand Down

0 comments on commit 02738c8

Please sign in to comment.