Optimize spans buffer insertion with eviction during insert - #1
Optimize spans buffer insertion with eviction during insert#1ron-x5labs wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe change removes repository GitHub Actions workflows and support scripts. It also updates span ingestion and buffering to preserve precise end timestamps, use Redis sorted sets, and cap merged buffers at 1,000 members. ChangesGitHub Actions removal
Span buffering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SpanConsumer
participant process_batch
participant SpanBuffer
participant Redis
SpanConsumer->>process_batch: decode span event
process_batch->>SpanBuffer: create Span with end_timestamp_precise
SpanBuffer->>Redis: store payload in sorted set
SpanBuffer->>Redis: scan sorted-set entries
Redis-->>SpanBuffer: return payload and score pairs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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. Comment |
![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 (3)
src/sentry/spans/buffer.py (1)
434-440: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the pipeline result pairing strict.
Ruff B905 flags the
zip()at Line 439. If the two sequences ever differ, plainzip()silently truncates the iteration. Addstrict=True, or use an explicit length check if the project target does not support it.Proposed change
- for key, (cursor, zscan_values) in zip(current_keys, results): + for key, (cursor, zscan_values) in zip(current_keys, results, strict=True):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sentry/spans/buffer.py` around lines 434 - 440, Update the zip call pairing current_keys with results in the pipeline processing loop to use strict pairing, adding strict=True if supported by the project’s Python target; otherwise perform an explicit length check before iterating so mismatched sequences are not silently truncated.Source: Linters/SAST tools
src/sentry/scripts/spans/add-buffer.lua (1)
62-64: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
max_segment_spansis ignored after eviction moved into Lua.
src/sentry/scripts/spans/add-buffer.lua#L62-L64: pass the configured limit into the script, or remove the public configuration.src/sentry/spans/buffer.py#L449-L449: keep the Python API and the Lua eviction contract consistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sentry/scripts/spans/add-buffer.lua` around lines 62 - 64, Make Lua eviction honor the configured max_segment_spans limit instead of the hardcoded 1000: update src/sentry/scripts/spans/add-buffer.lua lines 62-64 to receive and use that limit, and update src/sentry/spans/buffer.py line 449 to pass it while keeping the Python API and Lua eviction contract consistent.tests/sentry/spans/test_buffer.py (1)
126-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for sorted-set ranking and eviction.
These changes only add the required field to existing fixtures. Add a test with at least 1,001 spans and distinct
end_timestamp_precisevalues. Assert which 1,000 payloads remain after insertion. Also cover a late-parent merge with overlapping members.Also applies to: 195-221, 265-298, 343-368, 420-445, 500-500
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/sentry/spans/test_buffer.py` around lines 126 - 151, Add regression tests in the existing span buffer test cases using Span fixtures with at least 1,001 spans and distinct end_timestamp_precise values; insert them through the buffer and assert exactly which 1,000 payloads remain after sorted-set eviction. Also add coverage for a late-parent merge where members overlap, verifying the merged result and eviction ranking. Apply the required fixture field consistently across the referenced test scenarios.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/sentry/scripts/spans/add-buffer.lua`:
- Line 47: Update both ZUNIONSTORE calls in the add-buffer Lua script to specify
“AGGREGATE”, “MAX” instead of relying on the default SUM behavior, including the
call assigning span_count and the second call near line 53.
In `@src/sentry/spans/buffer.py`:
- Around line 197-199: Update the span buffering storage around the ZADD call
and its related ZUNIONSTORE/ZSCAN operations so sorted sets do not reuse
existing Redis set keys under the span-buf:s:* namespace. Use a versioned/new
namespace consistently, or add rollout migration/draining that converts existing
keys with valid scores before sorted-set commands run, preserving buffering and
draining during deployment.
---
Nitpick comments:
In `@src/sentry/scripts/spans/add-buffer.lua`:
- Around line 62-64: Make Lua eviction honor the configured max_segment_spans
limit instead of the hardcoded 1000: update
src/sentry/scripts/spans/add-buffer.lua lines 62-64 to receive and use that
limit, and update src/sentry/spans/buffer.py line 449 to pass it while keeping
the Python API and Lua eviction contract consistent.
In `@src/sentry/spans/buffer.py`:
- Around line 434-440: Update the zip call pairing current_keys with results in
the pipeline processing loop to use strict pairing, adding strict=True if
supported by the project’s Python target; otherwise perform an explicit length
check before iterating so mismatched sequences are not silently truncated.
In `@tests/sentry/spans/test_buffer.py`:
- Around line 126-151: Add regression tests in the existing span buffer test
cases using Span fixtures with at least 1,001 spans and distinct
end_timestamp_precise values; insert them through the buffer and assert exactly
which 1,000 payloads remain after sorted-set eviction. Also add coverage for a
late-parent merge where members overlap, verifying the merged result and
eviction ranking. Apply the required fixture field consistently across the
referenced test scenarios.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 63d7a8ec-6df5-4bc2-8cac-f3ff6de4a5d4
📒 Files selected for processing (39)
.github/workflows/acceptance.yml.github/workflows/backend.yml.github/workflows/bump-sentry-in-getsentry.yml.github/workflows/bump-version.yml.github/workflows/codecov_ats.yml.github/workflows/codecov_carryforward_reports.yml.github/workflows/codecov_per_test_coverage.yml.github/workflows/codeql.yml.github/workflows/development-environment.yml.github/workflows/enforce-license-compliance.yml.github/workflows/fast-revert.yml.github/workflows/frontend.yml.github/workflows/getsentry-dispatch.yml.github/workflows/jest-balance.yml.github/workflows/label-pullrequest.yml.github/workflows/lock.yml.github/workflows/meta-deploys-detect-change-type.yml.github/workflows/migrations.yml.github/workflows/openapi-diff.yml.github/workflows/openapi.yml.github/workflows/pre-commit.yml.github/workflows/publish-dockerhub.yml.github/workflows/react-to-product-owners-yml-changes.yml.github/workflows/release.yml.github/workflows/scripts/deploy.js.github/workflows/scripts/getsentry-dispatch-setup.github/workflows/scripts/getsentry-dispatch.js.github/workflows/scripts/migration-check.sh.github/workflows/scripts/wait-for-merge-commit.js.github/workflows/self-hosted.yml.github/workflows/sentry-pull-request-bot.yml.github/workflows/shuffle-tests.yml.github/workflows/sync-labels.ymlsrc/sentry/scripts/spans/add-buffer.luasrc/sentry/spans/buffer.pysrc/sentry/spans/consumers/process/factory.pytests/sentry/spans/consumers/process/test_consumer.pytests/sentry/spans/consumers/process/test_flusher.pytests/sentry/spans/test_buffer.py
💤 Files with no reviewable changes (33)
- .github/workflows/meta-deploys-detect-change-type.yml
- .github/workflows/sync-labels.yml
- .github/workflows/label-pullrequest.yml
- .github/workflows/jest-balance.yml
- .github/workflows/scripts/wait-for-merge-commit.js
- .github/workflows/scripts/getsentry-dispatch.js
- .github/workflows/fast-revert.yml
- .github/workflows/react-to-product-owners-yml-changes.yml
- .github/workflows/publish-dockerhub.yml
- .github/workflows/backend.yml
- .github/workflows/acceptance.yml
- .github/workflows/release.yml
- .github/workflows/shuffle-tests.yml
- .github/workflows/pre-commit.yml
- .github/workflows/enforce-license-compliance.yml
- .github/workflows/bump-sentry-in-getsentry.yml
- .github/workflows/self-hosted.yml
- .github/workflows/frontend.yml
- .github/workflows/codecov_carryforward_reports.yml
- .github/workflows/getsentry-dispatch.yml
- .github/workflows/openapi-diff.yml
- .github/workflows/codeql.yml
- .github/workflows/codecov_ats.yml
- .github/workflows/openapi.yml
- .github/workflows/codecov_per_test_coverage.yml
- .github/workflows/migrations.yml
- .github/workflows/scripts/migration-check.sh
- .github/workflows/lock.yml
- .github/workflows/bump-version.yml
- .github/workflows/scripts/getsentry-dispatch-setup
- .github/workflows/scripts/deploy.js
- .github/workflows/sentry-pull-request-bot.yml
- .github/workflows/development-environment.yml
| if not is_root_span and redis.call("scard", span_key) > 0 then | ||
| redis.call("sunionstore", set_key, set_key, span_key) | ||
| if not is_root_span and redis.call("zcard", span_key) > 0 then | ||
| span_count = redis.call("zunionstore", set_key, 2, set_key, span_key) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file ---'
cat -n src/sentry/scripts/spans/add-buffer.lua
printf '%s\n' '--- related buffer configuration and callers ---'
rg -n -C 4 'max_segment_spans|add-buffer|zunionstore|zpopmin|ZPOPMIN|ZUNIONSTORE' . \
-g '!node_modules' -g '!dist' -g '!build'Repository: ron-x5labs/sentry
Length of output: 8572
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- span buffer implementation ---'
sed -n '20,90p' src/sentry/spans/buffer.py
sed -n '112,230p' src/sentry/spans/buffer.py
printf '%s\n' '--- all writes and reads for span-buffer sorted sets ---'
rg -n -C 5 'zadd|ZADD|span-buf:s|end_timestamp|timestamp|payload' src/sentry/spans tests \
-g '*.py' -g '*.lua' -g '*.js' -g '*.ts' 2>/dev/null | head -n 500
printf '%s\n' '--- repository tests for add-buffer ---'
rg -n -C 8 'add-buffer|zunionstore|span_count|1000|max_segment_spans' tests src \
-g '*.py' -g '*.lua' | head -n 500Repository: ron-x5labs/sentry
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- focused buffer methods ---'
sed -n '185,285p' src/sentry/spans/buffer.py
sed -n '340,485p' src/sentry/spans/buffer.py
printf '%s\n' '--- focused tests and fixtures ---'
fd -i 'buffer|span' tests src/sentry/spans | grep -E 'buffer|span' | head -n 200
rg -n -C 5 'process_spans|flush_segments|end_timestamp_precise|zscan_iter|max_segment' tests/sentry tests -g '*span*' -g '*buffer*' 2>/dev/null | head -n 300
printf '%s\n' '--- Redis command references in project docs ---'
rg -n -C 3 'ZUNIONSTORE|AGGREGATE MAX|ZPOPMIN' . \
-g '*.lua' -g '*.py' -g '*.md' -g '*.rst' | head -n 200Repository: ron-x5labs/sentry
Length of output: 34692
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
# Read-only model of the Redis sorted-set operations used by add-buffer.lua.
# It checks the duplicate-member score behavior and the resulting ZPOPMIN order.
def zunion(left, right, aggregate):
out = dict(left)
for member, score in right.items():
if member not in out:
out[member] = score
elif aggregate == "SUM":
out[member] += score
elif aggregate == "MAX":
out[member] = max(out[member], score)
else:
raise ValueError(aggregate)
return out
def zpopmin(values, count):
return sorted(values.items(), key=lambda item: (item[1], item[0]))[:count]
left = {b"duplicate": 100.0, b"old": 101.0}
right = {b"duplicate": 100.0, b"new": 102.0}
for aggregate in ("SUM", "MAX"):
merged = zunion(left, right, aggregate)
evicted = zpopmin(merged, len(merged) - 2)
retained = set(merged) - {member for member, _ in evicted}
print(aggregate, merged, "evicted=", evicted, "retained=", sorted(retained))
assert zunion(left, right, "SUM")[b"duplicate"] == 200.0
assert zunion(left, right, "MAX")[b"duplicate"] == 100.0
assert b"duplicate" in (
set(zunion(left, right, "SUM"))
- {member for member, _ in zpopmin(zunion(left, right, "SUM"), 1)}
)
assert b"duplicate" not in (
set(zunion(left, right, "MAX"))
- {member for member, _ in zpopmin(zunion(left, right, "MAX"), 1)}
)
print("model assertions passed")
PYRepository: ron-x5labs/sentry
Length of output: 411
Use AGGREGATE MAX for both ZUNIONSTORE calls.
ZUNIONSTORE defaults to SUM. Duplicate payloads then receive summed timestamp scores, which can cause ZPOPMIN to retain them during eviction. Add "AGGREGATE", "MAX" at lines 47 and 53.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/sentry/scripts/spans/add-buffer.lua` at line 47, Update both ZUNIONSTORE
calls in the add-buffer Lua script to specify “AGGREGATE”, “MAX” instead of
relying on the default SUM behavior, including the call assigning span_count and
the second call near line 53.
| p.zadd( | ||
| set_key, {span.payload: span.end_timestamp_precise for span in subsegment} | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files 'src/sentry/spans/buffer.py' '*span*' '*redis*' | head -200
printf '%s\n' '--- buffer structure and key-related symbols ---'
ast-grep outline src/sentry/spans/buffer.py
rg -n -C 8 'zadd|zscan|sadd|smembers|spans|segment|key|namespace|max_segment_spans|drain' src/sentry/spans/buffer.py
printf '%s\n' '--- related references and tests ---'
rg -n -C 5 'SpansBuffer|max_segment_spans|zadd|zscan|segment.*key|key.*segment' . -g '*.py' -g '*.lua' -g '*.yml' -g '*.yaml' | head -500Repository: ron-x5labs/sentry
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- current Redis script ---'
cat -n src/sentry/scripts/spans/add-buffer.lua
printf '%s\n' '--- current buffer documentation and affected methods ---'
sed -n '28,70p;178,275p;343,485p' src/sentry/spans/buffer.py
printf '%s\n' '--- repository state and focused diff ---'
git status --short
git diff --stat
git diff -- src/sentry/spans/buffer.py src/sentry/scripts/spans/add-buffer.lua
printf '%s\n' '--- previous committed implementation ---'
parent="$(git rev-parse HEAD^)"
git show "$parent:src/sentry/spans/buffer.py" | sed -n '28,70p;178,275p;343,485p'
printf '%s\n' '--- previous Redis script ---'
git show "$parent:src/sentry/scripts/spans/add-buffer.lua" 2>/dev/null || true
printf '%s\n' '--- tests that directly create or inspect segment keys ---'
rg -n -C 4 'span-buf:s:|sadd|smembers|zadd|zscan|zrange|type\(' tests/sentry/spans src/sentry/scripts/spansRepository: ron-x5labs/sentry
Length of output: 33072
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- all segment-key migration or namespace references ---'
rg -n -i -C 3 'span-buf:s|migrat(e|ion)|zunionstore|sunionstore|zscan|sscan' \
src tests config .github -g '!*.json' -g '!*.lock' 2>/dev/null | head -500
printf '%s\n' '--- deployment and rollout references ---'
rg -n -i -C 3 'span buffer|spans buffer|buffered-spans|redis.*span|span.*redis' \
deploy* ops* infrastructure* src/sentry/conf* tests 2>/dev/null | head -400 || true
printf '%s\n' '--- focused static type-transition verifier ---'
python3 - <<'PY'
from pathlib import Path
import re
current = Path("src/sentry/spans/buffer.py").read_text()
previous = __import__("subprocess").check_output(
["git", "show", "HEAD^:src/sentry/spans/buffer.py"], text=True
)
current_lua = Path("src/sentry/scripts/spans/add-buffer.lua").read_text()
previous_lua = __import__("subprocess").check_output(
["git", "show", "HEAD^:src/sentry/scripts/spans/add-buffer.lua"], text=True
)
key_pattern = r'span-buf:s:\{[^}]+\}:[^"\']+'
current_keys = set(re.findall(key_pattern, current))
previous_keys = set(re.findall(key_pattern, previous))
print("same segment-key namespace:", bool(current_keys & previous_keys))
print("previous segment write command:", "SADD" if "p.sadd(" in previous else "not found")
print("current segment write command:", "ZADD" if "p.zadd(" in current else "not found")
print("previous Lua merge command:", "SUNIONSTORE" if "sunionstore" in previous_lua else "not found")
print("current Lua merge command:", "ZUNIONSTORE" if "zunionstore" in current_lua else "not found")
print("current segment read command:", "ZSCAN" if "p.zscan(" in current else "not found")
print("current Lua type probe:", "ZCARD" if 'redis.call("zcard"' in current_lua else "not found")
print("namespace changed:", current_keys.isdisjoint(previous_keys))
PYRepository: ron-x5labs/sentry
Length of output: 50374
Migrate or version existing span-buf:s:* keys before enabling sorted-set storage.
The previous implementation stores these keys as Redis sets. The current ZADD, ZUNIONSTORE, and ZSCAN commands use the same namespace. During a rolling deployment, existing set keys cause WRONGTYPE errors and prevent spans from being buffered or drained. Use a new namespace, migrate the keys with valid scores, or drain them before rollout.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/sentry/spans/buffer.py` around lines 197 - 199, Update the span buffering
storage around the ZADD call and its related ZUNIONSTORE/ZSCAN operations so
sorted sets do not reuse existing Redis set keys under the span-buf:s:*
namespace. Use a versioned/new namespace consistently, or add rollout
migration/draining that converts existing keys with valid scores before
sorted-set commands run, preserving buffering and draining during deployment.
ron-x5labs
left a comment
ron-x5labs
left a comment
There was a problem hiding this comment.
Code Review: Optimize spans buffer insertion with eviction during insert
Problem
This benchmark PR (recreated from getsentry#92393) aims to bound span-buffer memory by capping each segment ZSET at 1000 entries and evicting the oldest spans during insert, while preserving precise end timestamps as the ZSET score. It also strips the fork's CI/release workflows as benchmark scaffolding.
Solution Reviewed
Two parts: (1) bulk deletion of 33 files under .github/workflows/ (CI, release, CodeQL, license compliance, pre-commit, etc.); (2) spans buffer migration from Redis SET to ZSET — add-buffer.lua switches scard/sunionstore to zcard/zunionstore, adds zpopmin eviction when a segment exceeds 1000 entries, and buffer.py/factory.py thread end_timestamp_precise through as the ZSET score.
Summary
The ZSET + eviction design is sound and the core logic is correct, but there is one blocking robustness issue (end_timestamp_precise accessed as a required key) and several non-blocking gaps: dead/mismatched config, lost observability for eviction, a removed read-side safety net, a silent redirect-limit regression, and a stale has_root_span flag after eviction. The new eviction path is also untested. (Review posted as COMMENT because GitHub forbids REQUEST_CHANGES on the author's own PR — the inline blocking finding still stands.)
Files Reviewed
src/sentry/scripts/spans/add-buffer.lua— deeply reviewedsrc/sentry/spans/buffer.py— deeply reviewedsrc/sentry/spans/consumers/process/factory.py— deeply reviewedtests/sentry/spans/consumers/process/test_consumer.py— deeply reviewedtests/sentry/spans/consumers/process/test_flusher.py— deeply reviewedtests/sentry/spans/test_buffer.py— deeply reviewed.github/workflows/*(33 deleted files) — lightly reviewed (deletions)
Verification
tests/sentry/spans/test_buffer.py— skipped: integration tests require the full Sentry test harness (Django settings → Postgres) plus a real Redis cluster; the worktree has no venv and only Python 3.10 (Sentry requires 3.13). Not feasible to run locally here.- Static review: confirmed
end_timestamp_preciseis a real, widely-used field across the spans pipeline (process_segments/enrichment.py,message.py,convert.py, search attributes) and present insentry-kafka-schemas==1.3.6; confirmed the luaspan_countoverwrite is currently correct across all 4 merge branches but fragile; confirmed the 10000→1000 redirect-loop reduction and comment removal in the diff.
Notes on workflow deletions
The 33 deleted .github/workflows/* files remove CodeQL security scanning, backend/frontend/acceptance test matrices, license-compliance (FOSSA), and pre-commit enforcement. For a benchmark fork this is expected scaffolding, but if this branch is ever merged toward a production line of development these security/CI gates must be restored.
Suggestions
add-buffer.luaheader comment (line 9): the ARGS block documentspayloadas ARGV[1], but the code uses ARGV[1] asis_root_span(line 19); the payload is stored viaZADDinbuffer.pybefore this script runs. Update the comment to match the actual ARGV layout for anyone debugging the script.
Verdict
Recommend changes before merge — the required-key access can crash the consumer during rolling deploys, and the eviction/observability gaps should be addressed so span drops are visible and the dead config doesn't mislead future maintainers.
| parent_span_id=val.get("parent_span_id"), | ||
| project_id=val["project_id"], | ||
| payload=payload.value, | ||
| end_timestamp_precise=val["end_timestamp_precise"], |
There was a problem hiding this comment.
🔴 Blocking — required-key access can crash the consumer. val["end_timestamp_precise"] uses bracket access (raises KeyError), while the adjacent parent_span_id on line 138 correctly uses .get(). During a rolling deploy, in-flight Kafka messages produced before this field existed (or any transient schema mismatch / misconfigured producer) will raise KeyError, breaking ingestion for the entire batch and likely causing a retry loop. cast(SpanEvent, ...) is typing-only and provides no runtime validation. Use val.get("end_timestamp_precise") with a safe default (e.g. the batch timestamp already in scope) or add an explicit guard/drop.
| del cursors[key] | ||
| continue | ||
|
|
||
| payloads[key].extend(span for span, _ in zscan_values) |
There was a problem hiding this comment.
🟡 Non-blocking — removed read-side count safety net + dead config. The len(payloads[key]) > self.max_segment_spans guard (plus its drop + segment_span_count_exceeded metric + logger.error) was deleted here. The Lua eviction only runs on writes, so segments that pre-date the migration, or that transiently exceed 1000 during the TOCTOU window between process_spans Pipeline 1 (zadd) and Pipeline 2 (EVALSHA), can still be larger than 1000 at flush time and will load fully into memory. Keep a read-side cap (byte- or count-based) as a backstop. Additionally, max_segment_spans (default 1001, buffer.py:150) was the only consumer of this guard and is now dead code — its default (1001) also mismatches the Lua's hardcoded 1000. Either remove the parameter or wire it into the Lua script as an ARGV.
| end | ||
|
|
||
| if span_count > 1000 then | ||
| redis.call("zpopmin", set_key, span_count - 1000) |
There was a problem hiding this comment.
🟡 Non-blocking — eviction has zero observability. zpopmin silently drops the lowest-scored (oldest) spans, and its return value (popped elements/count) is discarded. No metric is incremented and no log is emitted. The old Python-side guard emitted spans.buffer.flush_segments.segment_span_count_exceeded + logger.error; that signal is now gone. Since eviction happens inline on every write, hot segments could be continuously dropping spans with no operator signal. Consider returning the evicted count to the caller and emitting a metric (e.g. spans.buffer.evicted).
| local redirect_depth = 0 | ||
|
|
||
| for i = 0, 10000 do -- theoretically this limit means that segment trees of depth 10k may not be joined together correctly. | ||
| for i = 0, 1000 do |
There was a problem hiding this comment.
🟡 Non-blocking — silent redirect-limit regression. The loop bound was reduced from 10000 to 1000 (10×), and the explanatory comment ("theoretically this limit means that segment trees of depth 10k may not be joined together correctly") was removed. Segment trees deeper than 1000 will now silently fail to join correctly. The PR body mentions the reduction but gives no rationale for this specific bound. If 1000 is intentional, keep a comment documenting the new limit and its tradeoff; otherwise confirm no real traces exceed depth 1000.
| redis.call("zpopmin", set_key, span_count - 1000) | ||
| end | ||
|
|
||
| local has_root_span_key = string.format("span-buf:hrs:%s", set_key) |
There was a problem hiding this comment.
🟡 Non-blocking — stale has_root_span flag after eviction. zpopmin (line 63) evicts the lowest-scored spans, i.e. oldest end_timestamp_precise — the root/segment span is typically the earliest and most likely to be evicted. But the has_root_span_key flag (lines 66-69) is set/retained based on is_root_span or the pre-existing flag value and is never invalidated when a root span is evicted. The segment then flushes on the shorter span_buffer_root_timeout_secs despite no longer containing a root span. This is conservative (flushes sooner) rather than lossy, but it violates the flag's invariant.
| if not is_root_span and redis.call("scard", span_key) > 0 then | ||
| redis.call("sunionstore", set_key, set_key, span_key) | ||
| if not is_root_span and redis.call("zcard", span_key) > 0 then | ||
| span_count = redis.call("zunionstore", set_key, 2, set_key, span_key) |
There was a problem hiding this comment.
🟡 Non-blocking — fragile span_count accounting. When both merges fire, span_count from the first zunionstore (line 47) is unconditionally overwritten by the second (line 53). This is currently correct because the second merge's destination (set_key) already includes the first merge's results, so its return is the true total. But it's fragile: the zcard fallback on line 59 only fires when span_count == 0, so a stale intermediate count would silently mis-size eviction. Consider calling zcard(set_key) unconditionally before the span_count > 1000 check instead of relying on the last zunionstore return.
1 participant
Benchmark PR recreated from getsentry#92393
Summary by CodeRabbit