Skip to content

DLQ testing with options feature flag #82283

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from

Conversation

kneeyo1
Copy link
Contributor

@kneeyo1 kneeyo1 commented Dec 18, 2024

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

Copy link

sentry-io bot commented Dec 18, 2024

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: src/sentry/ingest/consumer/processors.py

Function Unhandled Issue
process_event Retriable: [Errno 111] Connection refused ingest_...
Event Count: 26

Did you find this useful? React with a 👍 or 👎

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Dec 18, 2024
consumer_type == ConsumerType.Transactions or data.get("type") == "transaction"
) and options.get("transactions.stale_dlq_enabled"):
# check timestamp
timestamp = message.get("timestamp") if message.get("timestamp") else start_time
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure if message.get('timesstamp") works need to double check

Copy link
Contributor

Choose a reason for hiding this comment

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

i think it'll be data.get("timestamp") instead

Copy link
Contributor

Choose a reason for hiding this comment

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

i found in the schema repo that "timestamp" field is on the same level as "type". And in this file the field type is being accesed like data.get("type")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you!

# check timestamp
timestamp = message.get("timestamp") if message.get("timestamp") else start_time

if timestamp and datetime.datetime.now() - timestamp > options.get(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these times probably need to be normalized

Copy link
Contributor

Choose a reason for hiding this comment

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

what do you mean normalized?

raise InvalidMessage(
message.get("partition"),
message.get("offset"),
reason="Stale Message",
Copy link
Contributor

Choose a reason for hiding this comment

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

let's not do a space? idk spaces usually scare me in strings

consumer_type == ConsumerType.Transactions or data.get("type") == "transaction"
) and options.get("transactions.stale_dlq_enabled"):
# check timestamp
timestamp = message.get("timestamp") if message.get("timestamp") else start_time
Copy link
Contributor

Choose a reason for hiding this comment

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

i think it'll be data.get("timestamp") instead

consumer_type == ConsumerType.Transactions or data.get("type") == "transaction"
) and options.get("transactions.stale_dlq_enabled"):
# check timestamp
timestamp = message.get("timestamp") if message.get("timestamp") else start_time
Copy link
Contributor

Choose a reason for hiding this comment

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

i found in the schema repo that "timestamp" field is on the same level as "type". And in this file the field type is being accesed like data.get("type")

@kneeyo1
Copy link
Contributor Author

kneeyo1 commented Dec 18, 2024

closing because #82322

@kneeyo1 kneeyo1 closed this Dec 18, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jan 3, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants