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(replay): Config new ingest-feedback-events topic #227

Merged
merged 20 commits into from Mar 12, 2024

Conversation

aliu3ntry
Copy link
Member

@aliu3ntry aliu3ntry commented Mar 1, 2024

https://getsentry.atlassian.net/browse/OPS-5317
getsentry/sentry#66100

New kafka topic for user feedback events, sourced from feedback widget or crash report modal. Schema is identical to ingest-events since feedback is currently in this topic.

  • Pipeline = user-feedback (new), since this product does not depend on, and is not depended on, by other topics.
  • Cluster = replays, since replay team owns user feedback. We expect the volume to be too low to justify its own cluster.

Schema references (structure of the message payload):

Also see getsentry/sentry#66484

@aliu3ntry aliu3ntry requested a review from a team as a code owner March 1, 2024 22:23
@aliu3ntry aliu3ntry requested review from lynnagara, bmckerry and a team March 1, 2024 22:27
topics/ingest-feedbacks.yaml Outdated Show resolved Hide resolved
@@ -0,0 +1,17 @@
topic: ingest-feedbacks
pipeline: replays
Copy link
Member

Choose a reason for hiding this comment

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

is it actually part of the replays product? isn't this just general user feedback which may or may not be associated with a replay?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes it's not always associated. can you elaborate on what "pipeline" is for?

The idea was to use the same cluster/infrastructure, not cause the products are dependent, but because the same team owns them. Also to save costs

Copy link
Member

@lynnagara lynnagara Mar 4, 2024

Choose a reason for hiding this comment

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

it provides an view on what features are linked together and how data flows through the system.

As an example, if ingest-events is broken, then data is not going through to events and other related topics as well as they feed into each other.

It's got nothing to do with which teams are working on what at sentry at any given point it time (which unfortunately seems to change every quarter).

I think you could probably just make this errors here, since it's moving from the shared errors topic today.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, thanks for the explanation! what does the errors pipeline's dependency chain look like?
I think part of the reason we want to move it off that topic is because feedback is not dependent -- see motivation in getsentry/sentry#66100. Thoughts on making a new pipeline for this topic?

Also, can we still have it on the same cluster, if it's a different pipeline? And does cluster need to be defined in some config file?

Copy link
Member

Choose a reason for hiding this comment

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

There's quite a few things in the errors pipeline (you can search for the files in this director that contain errors if you want to see some example topics). New pipeline sounds fine -- you can just change it here to user-feedback or whatever you like here, there's nothing else to do.

Yes, the cluster topology is independent and can always be mapped to whatever kafka cluster you like per environment later.

topics/ingest-feedbacks.yaml Outdated Show resolved Hide resolved
topics/ingest-feedbacks.yaml Outdated Show resolved Hide resolved
Comment on lines 16 to 17
topic_creation_config:
message.timestamp.type: LogAppendTime
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need this, you can remove the whole topic_creation_config block unless you have some special configuration you want to enforce

@aliu3ntry aliu3ntry requested a review from a team as a code owner March 4, 2024 22:00
@aliu3ntry aliu3ntry force-pushed the add-ingest-feedbacks-topic-config branch from 1399c42 to 317c598 Compare March 5, 2024 23:01
@aliu3ntry aliu3ntry changed the title feat(replay): Add ingest feedbacks topic config feat(replay): Config new ingest-feedback-events topic Mar 5, 2024
@aliu3ntry
Copy link
Member Author

aliu3ntry commented Mar 5, 2024

Summary of last commit:

  • move to new 'user-feedback' pipeline
  • add a first draft schema generated from a widget feedback (captured by chrome dev tools) and https://github.com/glideapps/quicktype (no manual modifications). Prob needs field descriptions.
  • rename to 'ingest-feedback-events' since 'FeedbackEvent' seems to be the corresponding type in Django + SDK/typescript

Open to discussion on the renaming! Not sure if @bmckerry has already provisioned ingest-feedbacks, and if so, is it rename-able?

CODEOWNERS Outdated Show resolved Hide resolved
@aliu3ntry aliu3ntry requested a review from a team March 6, 2024 18:34
aliu3ntry and others added 12 commits March 7, 2024 10:49
https://getsentry.atlassian.net/browse/OPS-5317
getsentry/sentry#66100

Pipeline/cluster = replays, since replay team owns user feedback. We expect the volume to be too low to justify its own cluster.
+small change to descrip
- move to new 'user-feedback' pipeline
- add an initial schema generated from a widget feedback (captured by chrome dev tools) and https://github.com/glideapps/quicktype
- rename to 'ingest-feedback-events' since 'FeedbackEvent' seems to be the corresponding type in Django + SDK/typescript
…_email, message, name, source}}

- all fields have additionalProperties = true, making schema totally flexible and backward/forward-compatible
- rename examples folder
@aliu3ntry aliu3ntry force-pushed the add-ingest-feedbacks-topic-config branch from ecd0107 to 6c579a7 Compare March 7, 2024 18:51
@@ -0,0 +1,67 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

How come these examples don't have project_id? Isn't that a required field in user feedback? It's not on the schema either.

aliu3ntry and others added 5 commits March 7, 2024 12:08
- turns out relay wraps the previous schema in the msgpack 'payload' field, as an encoded JSON string
- new schema is almost identical to ingest-events. Bytes is a not a valid jsonschema type so we omit type from the payload
@aliu3ntry
Copy link
Member Author

Turns out relay wraps my previous schema in the msgpack 'payload' field, as an encoded JSON string. So the new schema is identical to ingest-events. Bytes is a not a valid jsonschema type so we have to omit type from 'payload'

@aliu3ntry aliu3ntry merged commit e74bfe4 into main Mar 12, 2024
14 checks passed
@aliu3ntry aliu3ntry deleted the add-ingest-feedbacks-topic-config branch March 12, 2024 00:29
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