Skip to content

feat(dlq): add dlq support (no-op)#277

Open
victoria-yining-huang wants to merge 17 commits intomainfrom
vic/add_dlq
Open

feat(dlq): add dlq support (no-op)#277
victoria-yining-huang wants to merge 17 commits intomainfrom
vic/add_dlq

Conversation

@victoria-yining-huang
Copy link
Copy Markdown
Contributor

@victoria-yining-huang victoria-yining-huang commented Mar 20, 2026

ticket

PR1 (this): add arroyo dlq support that can reach rust-arroyo - noop
PR2: polish deployment config yaml (json validation), default config - noop
PR3: wire everything together, end to end testing
PR4: enable by default, deploy some default topics

@victoria-yining-huang victoria-yining-huang requested a review from a team as a code owner March 20, 2026 19:50
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 20, 2026

Semver Impact of This PR

🟡 Minor (new features)

📋 Changelog Preview

This is how your changes will appear in the changelog.
Entries from this PR are highlighted with a left border (blockquote style).


New Features ✨

  • (dlq) Add dlq support (no-op) by victoria-yining-huang in #277

🤖 This preview updates automatically when you update the PR.

Comment on lines +17 to +21

enabled: bool
topic: str
producer_config: "KafkaProducerConfig"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The Python adapter in rust_arroyo.py doesn't read the dlq configuration or pass it to the Rust ArroyoConsumer, rendering the DLQ feature non-functional.
Severity: CRITICAL

Suggested Fix

Update rust_arroyo.py to read the dlq configuration from the source config. If present, construct the corresponding Rust DlqConfig object and pass it as the dlq_config argument when initializing the ArroyoConsumer. Add integration tests to verify the DLQ functionality.

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: sentry_streams/sentry_streams/config_types.py#L17-L21

Potential issue: The pull request adds Dead Letter Queue (DLQ) support, with a
`DlqConfig` defined in Python and the Rust consumer expecting a `dlq_config` parameter.
However, the Python adapter in `rust_arroyo.py` that instantiates the `ArroyoConsumer`
never reads the `dlq` key from the configuration dictionary and does not pass it during
initialization. As a result, any user-provided DLQ configuration will be silently
ignored, and the DLQ functionality will not work. Invalid messages will be dropped
instead of being routed to the configured dead-letter topic.

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Autofix Details

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Python type stub missing new DLQ class and parameter
    • Updated rust_streams.pyi to add PyDlqConfig and the optional dlq_config argument on ArroyoConsumer.__init__ to match the Rust-exposed interface.

Create PR

Or push these changes by commenting:

@cursor push cb8d770063
Preview (cb8d770063)
diff --git a/sentry_streams/sentry_streams/rust_streams.pyi b/sentry_streams/sentry_streams/rust_streams.pyi
--- a/sentry_streams/sentry_streams/rust_streams.pyi
+++ b/sentry_streams/sentry_streams/rust_streams.pyi
@@ -41,6 +41,12 @@
         override_params: Mapping[str, str],
     ) -> None: ...
 
+class PyDlqConfig:
+    topic: str
+    producer_config: PyKafkaProducerConfig
+
+    def __init__(self, topic: str, producer_config: PyKafkaProducerConfig) -> None: ...
+
 class PyMetricConfig:
     def __init__(
         self,
@@ -105,6 +111,7 @@
         schema: str | None,
         metric_config: PyMetricConfig | None = None,
         write_healthcheck: bool = False,
+        dlq_config: PyDlqConfig | None = None,
     ) -> None: ...
     def add_step(self, step: RuntimeOperator) -> None: ...
     def run(self) -> None: ...

This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.

@victoria-yining-huang victoria-yining-huang changed the title feat(dlq): add dlq support rust side feat(dlq): add dlq support (no-op) Mar 23, 2026
Adds DLQ configuration support throughout the streaming platform:
- Add PyDlqConfig type stub and build_dlq_config() helper
- Wire DLQ config from StreamSource to Rust ArroyoConsumer
- Support DLQ override from YAML deployment configuration
- Add comprehensive test coverage for DLQ functionality

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Copy link
Copy Markdown
Collaborator

@fpacifici fpacifici left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my comments on the unit tests and on the defaults.

Also please adjust the example pipelines to set up the DLQ. Since this cannot be tested with unit test consider adapting https://github.com/getsentry/streams/blob/main/sentry_streams/integration_tests/test_example_pipelines.py

/// Builds the DLQ policy if dlq_config is provided.
/// Returns None if DLQ is not configured.
fn build_dlq_policy(&self) -> Option<DlqPolicy<KafkaPayload>> {
match &self.dlq_config {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making this a standalone funciton also allows you not to have this case for when the config is not provided.

/// When provided, invalid messages will be sent to the DLQ topic.
#[pyclass]
#[derive(Debug, Clone)]
pub struct PyDlqConfig {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why PyDlqConfig rather than DlqConfig? Is there a rust version that we need to distinguish from ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just saw that it's a naming convention we have in the repo, like PyKafkaConsumerConfig. I assume it means "this Rust struct is meant for coming from Python"

Comment on lines +264 to +265
topic=dlq_data["topic"],
bootstrap_servers=dlq_data["bootstrap_servers"],
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the user wants to override only the topic name and leave the bootstrap_servers as they are?
Also should we make the bootstrap_servers default to the same one we use for the StreamingSource ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added individual field overriding ability

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default value i was planning on handling in the next PR, see my PR description

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 3 total unresolved issues (including 2 from previous reviews).

Fix All in Cursor

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants