Skip to content
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

feat(feedback): create a KafkaMessage type for user feedback #3467

Closed
wants to merge 5 commits into from

Conversation

aliu3ntry
Copy link
Member

Now that feedback is moving to its own topic ingest-feedback-events (getsentry/sentry#66100), it's a good time to split it off from EventKafkaMessage.

Will be used by #3403. Attachments associated with a feedback are, and will continue to be, sent separately to the attachments topic. So this field is unnecessary.

Attachments are optional for the downstream ingest consumer, for all msgs read from ingest-events: https://github.com/getsentry/sentry/blob/40008bc7a981f8f577d506120a1da0ea0dc60cf0/src/sentry/ingest/consumer/processors.py#L68

@aliu3ntry aliu3ntry requested a review from a team as a code owner April 19, 2024 23:05
@aliu3ntry aliu3ntry requested a review from a team April 19, 2024 23:05
Copy link
Member

@iker-barriocanal iker-barriocanal left a comment

Choose a reason for hiding this comment

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

Thanks for working on the PR! Let us know in #discuss-ingest if there are any difficulties in making the PR work or tests pass.

assert event["type"] == "feedback"


# /// If there is a UserReportV2 item, . The attachments should be kept in
Copy link
Member

Choose a reason for hiding this comment

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

Is the commented code below valid? If not, let's remove it.

42, extra={"config": {"features": ["organizations:user-feedback-ingest"]}}
)
# mini_sentry.set_global_config_option("feedback.ingest-topic.rollout-rate", 1.0)
# TODO: make and set a FF here
Copy link
Member

Choose a reason for hiding this comment

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

Are the TODOs in this test part of the scope of this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, that's weird, the changes to this file are all for another PR, not sure how I included them here. Will revert

Copy link
Member

@JoshFerge JoshFerge left a comment

Choose a reason for hiding this comment

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

lgtm

@aliu3ntry
Copy link
Member Author

Moving to #3403 to make merging this easier (good for code structuring and passing lint CI)

@aliu3ntry aliu3ntry closed this Apr 22, 2024
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.

None yet

3 participants