feat(replay): add kafka config for new ingest-feedbacks consumer #66169

aliu3ntry · 2024-03-01T21:45:03Z

Linked to #66100 and
https://getsentry.atlassian.net/browse/OPS-5317

may have to rename to ingest-feedback-events

May have to wait on:
#66288
#66381

src/sentry/conf/server.py

lynnagara · 2024-03-01T22:15:44Z

src/sentry/consumers/__init__.py

@@ -269,6 +269,14 @@ def ingest_events_options() -> list[click.Option]:
            "consumer_type": "events",
        },
    },
+    "ingest-feedbacks": {
+        "topic": settings.KAFKA_INGEST_FEEDBACKS,
+        "strategy_factory": "sentry.ingest.consumer.factory.IngestStrategyFactory",


Each consumer should have it's own strategy. The ingest strategy is already doing far more than it should and we should not add additional logic to it.

Any tips/references for how to write a new strategy? In ingest/consumer/factory.py, right

The directory structure in sentry is a bit misleading, but the ingest module really just refers to the errors/transactions ingest consumer today and not any of the rest of the piplines. I would suggest keeping it either entirely separate, or inside a new module ingest.user_feedback or something like that. See https://getsentry.github.io/arroyo/ for docs how to write a strategy.

@lynnagara We're not adding additional logic to it. This behavior already exists in that consumer. We're just moving it to a new topic and new consumer infrastructure. I agree we should split out this product into its own consumer. Can that be handled in a follow-up PR?

Ah makes sense, I thought it was totally new functionality.

codecov · 2024-03-01T22:24:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.29%. Comparing base (f58252f) to head (f0f614a).
Report is 12 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #66169      +/-   ##
==========================================
+ Coverage   84.27%   84.29%   +0.01%     
==========================================
  Files        5304     5304              
  Lines      237045   237075      +30     
  Branches    41026    41031       +5     
==========================================
+ Hits       199776   199834      +58     
+ Misses      37050    37022      -28     
  Partials      219      219

Files	Coverage Δ
src/sentry/conf/server.py	`90.08% <100.00%> (+0.01%)`	⬆️
src/sentry/conf/types/kafka_definition.py	`100.00% <100.00%> (ø)`
src/sentry/consumers/__init__.py	`71.73% <ø> (ø)`

... and 11 files with indirect coverage changes

lynnagara · 2024-03-06T22:59:21Z

src/sentry/consumers/__init__.py

+        "strategy_factory": "sentry.ingest.consumer.factory.IngestStrategyFactory",
+        "click_options": ingest_events_options(),
+        "static_args": {
+            "consumer_type": "events",


I think you might want to make this a new type. We don't really want to run all the events logic and configuration (even if they share the bulk of code in the initial implementation), and i'm guessing this will diverge in the future?

…re flag (#66257) removes flag declaration and all refs getsentry/team-replay#387

Was using TSResult here which is wrong, it's a dict w/ data or a dict of dicts if it's a group-by. The presence of the data key should be indicative. Fixes SENTRY-2V84

### Summary If there are any problems with the split decision this will cause it to always re-run the code and save the new decision,

<img width="1035" alt="Screenshot 2024-03-03 at 9 36 22 PM" src="https://github.com/getsentry/sentry/assets/60121741/860a6370-b1aa-41dc-b5c8-b62b9778ffa5"> --------- Co-authored-by: Abdullah Khan <abdullahkhan@PG9Y57YDXQ.local>

This PR adds the current user to the header of the replay in the issue details page: Before: ![Screenshot 2024-03-04 at 1 15 37 PM](https://github.com/getsentry/sentry/assets/8533851/614047aa-e0da-4520-91d0-5d71f22f8cd9) After: ![Screenshot 2024-03-04 at 1 15 31 PM](https://github.com/getsentry/sentry/assets/8533851/4592b55d-ac83-4448-8af5-06988dfc5aa1)

…or environment (#66265) These constraints are causing migrations to fail in getsentry, where we've set up the router to move these tables to a separate database.

We need to support deleted fields on Django model schemas for a number of self-hosted released (currently 2). This means that even if we delete a field on HEAD, we still need to make the import script aware that the field could exit on imported JSON, and pre-process that JSON in such a way that the field is removed in cases where the JSON was generated by an old version that still contained the field. There is no purpose in testing this code standalone. It passes tests in #65923, which is the first actual use of this shimming mechanism.

Just moving 3 consumers as a first step, will migrate the rest as a follow up

#66277)

Toggles off INP

failure_issue_threshold will never be falsey due because of this condition here [0] [0]: https://github.com/getsentry/sentry/blob/11e039354141a8487776be19d7535d0da2ee53ce/src/sentry/monitors/logic/mark_failed.py#L44-L45

github-actions · 2024-03-06T23:14:54Z

🚨 Warning: This pull request contains Frontend and Backend changes!

It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently.

Have questions? Please ask in the #discuss-dev-infra channel.

aliu3ntry · 2024-03-06T23:28:09Z

my bad 😂. reopened at #66484

feat(replay): add kafka config for new ingest-feedbacks consumer

62dd685

aliu3ntry requested a review from a team as a code owner March 1, 2024 21:45

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Mar 1, 2024

Merge branch 'master' into aliu/ingest-feedbacks-config

78cedd6

vercel bot deployed to Preview March 1, 2024 21:48 View deployment

aliu3ntry requested review from a team March 1, 2024 21:57

lynnagara reviewed Mar 1, 2024

View reviewed changes

src/sentry/conf/server.py Outdated Show resolved Hide resolved

lynnagara reviewed Mar 1, 2024

View reviewed changes

Merge branch 'master' into aliu/ingest-feedbacks-config

2552f1a

vercel bot deployed to Preview March 4, 2024 18:54 View deployment

Merge branch 'master' into aliu/ingest-feedbacks-config

7292129

vercel bot deployed to Preview March 4, 2024 22:02 View deployment

lynnagara mentioned this pull request Mar 4, 2024

feat(replay): Config new ingest-feedback-events topic getsentry/sentry-kafka-schemas#227

Merged

add ingest-feedbacks to KAFKA_TOPIC_TO_CLUSTER and kafka_definition

f0f614a

vercel bot deployed to Preview March 5, 2024 01:46 View deployment

lynnagara reviewed Mar 6, 2024

View reviewed changes

aliu3ntry and others added 12 commits March 6, 2024 15:10

feat(replay): remove organizations:session-replay-event-linking featu…

4dc60ad

…re flag (#66257) removes flag declaration and all refs getsentry/team-replay#387

fix(metrics-extraction): Fix meta lookup in factory (#66256)

69e2a34

Was using TSResult here which is wrong, it's a dict w/ data or a dict of dicts if it's a group-by. The presence of the data key should be indicative. Fixes SENTRY-2V84

fix(metrics-extraction): Allow overriding split decision (#66259)

9d26087

### Summary If there are any problems with the split decision this will cause it to always re-run the code and save the new decision,

feat(autofix): timeout with autofix, query state (#66241)

35a45b3

fix(crons): Remove db constraint from historical migrations for monit…

8763274

…or environment (#66265) These constraints are causing migrations to fail in getsentry, where we've set up the router to move these tables to a separate database.

ref: Move topics to new-style of Kafka topic definition (#66271)

8823af4

Just moving 3 consumers as a first step, will migrate the rest as a follow up

fix(replay): delete flaky test test_get_replays_dead_rage_click_cutoff (

99e584e

#66277)

fix(performance): Turn off INP on the browser integration sdk (#66279)

81f6adc

Toggles off INP

ref(crons): Remove unused code branch in mark_failed (#66284)

83e2e1c

failure_issue_threshold will never be falsey due because of this condition here [0] [0]: https://github.com/getsentry/sentry/blob/11e039354141a8487776be19d7535d0da2ee53ce/src/sentry/monitors/logic/mark_failed.py#L44-L45

aliu3ntry requested review from a team as code owners March 6, 2024 23:14

aliu3ntry requested a review from a team March 6, 2024 23:14

aliu3ntry requested review from a team as code owners March 6, 2024 23:14

github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Mar 6, 2024

aliu3ntry closed this Mar 6, 2024

vercel bot deployed to Preview March 6, 2024 23:16 View deployment

aliu3ntry deleted the aliu/ingest-feedbacks-config branch March 15, 2024 23:39

github-actions bot locked and limited conversation to collaborators Mar 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(replay): add kafka config for new ingest-feedbacks consumer #66169

feat(replay): add kafka config for new ingest-feedbacks consumer #66169

aliu3ntry commented Mar 1, 2024 •

edited

lynnagara Mar 1, 2024 •

edited

aliu3ntry Mar 1, 2024

lynnagara Mar 1, 2024

cmanallen Mar 4, 2024 •

edited

lynnagara Mar 4, 2024

codecov bot commented Mar 1, 2024 •

edited

lynnagara Mar 6, 2024 •

edited

github-actions bot commented Mar 6, 2024

aliu3ntry commented Mar 6, 2024

feat(replay): add kafka config for new ingest-feedbacks consumer #66169

feat(replay): add kafka config for new ingest-feedbacks consumer #66169

Conversation

aliu3ntry commented Mar 1, 2024 • edited

lynnagara Mar 1, 2024 • edited

Choose a reason for hiding this comment

aliu3ntry Mar 1, 2024

Choose a reason for hiding this comment

lynnagara Mar 1, 2024

Choose a reason for hiding this comment

cmanallen Mar 4, 2024 • edited

Choose a reason for hiding this comment

lynnagara Mar 4, 2024

Choose a reason for hiding this comment

codecov bot commented Mar 1, 2024 • edited

Codecov Report

lynnagara Mar 6, 2024 • edited

Choose a reason for hiding this comment

github-actions bot commented Mar 6, 2024

aliu3ntry commented Mar 6, 2024

aliu3ntry commented Mar 1, 2024 •

edited

lynnagara Mar 1, 2024 •

edited

cmanallen Mar 4, 2024 •

edited

codecov bot commented Mar 1, 2024 •

edited

lynnagara Mar 6, 2024 •

edited