feat(consumer): add a DLQ-by-age load-shedding lever#8167
Open
onewland wants to merge 3 commits into
Open
Conversation
Introduce DlqByAge, a stateless arroyo strategy for the Rust consumer that, during a backlog, routes a configurable proportion of messages older than a threshold (by Kafka broker timestamp) to the DLQ using the standard InvalidMessage mechanism. The rest are forwarded untouched, so a backlogged consumer can catch up to fresh data while the parked backlog is backfilled from the DLQ later. Unlike the removed BLQRouter, this has no state machine, never panics the consumer, and owns no producer -- it relies entirely on arroyo's existing DLQ buffer + policy. It is only inserted into the pipeline when a DLQ topic is configured, because without a DLQ policy an InvalidMessage is logged and silently dropped (losing data). Configured per-message via sentry-options so it is runtime-tunable: - consumer.dlq_by_age_sample_rate_by_storage: dict of storage -> fraction [0,1] of too-old messages to dead-letter; defaults to 0 (disabled). - consumer.dlq_by_age_threshold_seconds: age cutoff, default 3600s. Ships disabled (no storage enabled, rate defaults to 0). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Age-based DLQ routing is an expected load-shedding outcome, not an error, so it should not create Sentry issues. arroyo logs an `Invalid` InvalidMessage at ERROR level (which the tracing->Sentry layer captures as an issue) but logs `Ignored` at DEBUG. Both reasons produce to the DLQ identically, so switching DlqByAge to `InvalidMessageReason::Ignored` keeps these out of Sentry while still dead-lettering the messages. Same intent as the SilencedDLQMessage path added in #8012. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Explain in the schema description what the boundary values mean: 0.0 routes none (disabled), 1.0 routes every too-old message, and a fractional value routes that proportion and forwards the rest. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| .and_then(|o| o.get("consumer.dlq_by_age_threshold_seconds").ok()) | ||
| .and_then(|v| v.as_i64()) | ||
| .filter(|s| *s > 0) | ||
| .map(TimeDelta::seconds) |
Contributor
There was a problem hiding this comment.
Bug: The threshold() method can panic due to an unvalidated, large configuration value from consumer.dlq_by_age_threshold_seconds, causing a consumer crash loop.
Severity: MEDIUM
Suggested Fix
Use the fallible TimeDelta::try_seconds() function instead of TimeDelta::seconds() to handle potentially large values without panicking. Additionally, consider adding a maximum constraint to the consumer.dlq_by_age_threshold_seconds option in the JSON schema to prevent invalid configurations.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: rust_snuba/src/strategies/dlq_by_age.rs#L91
Potential issue: The `threshold()` method reads the
`consumer.dlq_by_age_threshold_seconds` configuration value and passes it directly to
`TimeDelta::seconds()`. This function will panic if the input value is excessively large
(greater than `i64::MAX / 1_000`). The configuration schema does not enforce a maximum
value, and the code only validates that the number is positive. Because this method is
called for every message processed, a misconfiguration with a very large number would
cause the consumer to enter a persistent crash loop, preventing it from processing any
messages.
Did we get this right? 👍 / 👎 to inform future reviews.
Contributor
Author
There was a problem hiding this comment.
This doesn't concern me
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
DlqByAge, a stateless arroyo strategy for the Rust consumer that sheds load during a backlog. When enabled, it routes a configurable proportion of messages older than a threshold (by Kafka broker timestamp) to the DLQ via the standardInvalidMessagemechanism. The rest are forwarded untouched — so a backlogged consumer catches up to fresh data while the parked backlog is backfilled from the DLQ later.This replaces the removed BLQ mechanism with something far simpler: no state machine, no consumer panic, no separate producer. It leans entirely on arroyo's existing DLQ buffer + policy.
How it works
BLQRouterwas), before decode/process, so dropped messages cost almost nothing.now - broker_ts <= threshold→ forward. Else rollrand < rate: hit →Err(SubmitError::InvalidMessage)(→ DLQ); miss → forward.dlq_configuredflag fromconsumer.rs). Without a DLQ policy, arroyo logs anInvalidMessageand silently drops it — which would lose data — so we never wire the strategy in that case.Kept out of Sentry
Age-based routing is an expected load-shedding outcome, not an error. Dead-lettered messages use
InvalidMessageReason::Ignored, so arroyo logs them at DEBUG (not ERROR) and the tracing→Sentry layer (logging.rs) does not turn them into Sentry issues. Both reasons produce to the DLQ identically — only the log level differs. This is the same intent as theSilencedDLQMessagepath from #8012.Configuration (sentry-options, read per-message, runtime-tunable)
consumer.dlq_by_age_sample_rate_by_storage{storage: number}{}consumer.dlq_by_age_threshold_seconds3600Ships disabled — no storage is enabled and the rate defaults to 0, so this is an inert passthrough until an operator opts a storage in.
Metrics:
dlq_by_age.stale_seenanddlq_by_age.routed_to_dlq(storage is already a global metric tag).Test plan
cargo test --lib dlq_by_age— 6 unit tests pass: disabled-by-default, explicit rate 0, fresh-forwarded, rate 1 routes all stale, per-storage scoping, and a partial-rate split. Each DLQ'd message is asserted to carryIgnored.cargo clippy --all-targetsclean;cargo fmt --checkclean.Follow-up (PR 3, ops)
Enable per storage via sentry-options and watch
dlq_by_age.routed_to_dlq+ DLQ topic lag.🤖 Generated with Claude Code