feat(feedback): create kafka topic and consumer for ingest-feedback #3060
Conversation
| @@ -250,7 +249,8 @@ services: | |||
| # Override the entrypoint in order to avoid using envvars for config. | |||
| # Futz with settings so we can keep mmdb and conf in same dir on host | |||
| # (image looks for them in separate dirs by default). | |||
| entrypoint: ["/usr/bin/geoipupdate", "-d", "/sentry", "-f", "/sentry/GeoIP.conf"] | |||
There was a problem hiding this comment.
These were from my vscode formatter..
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #3060 +/- ##
=======================================
Coverage 99.01% 99.01%
=======================================
Files 3 3
Lines 203 203
=======================================
Hits 201 201
Misses 2 2 ☔ View full report in Codecov by Sentry. |
docker-compose.yml
Outdated
| @@ -363,6 +363,9 @@ services: | |||
| transactions-consumer: | |||
| <<: *sentry_defaults | |||
| command: run consumer ingest-transactions --consumer-group ingest-consumer | |||
| ingest-feedback-events: | |||
There was a problem hiding this comment.
nit: could you change this to feedback-events-consumer? It matches the naming convention of the other ingest consumers
There was a problem hiding this comment.
Sure, should it be changed in create-kafka-topics.sh too? It's named ingest-feedback-events in prod so I was worried bout keeping consistent with that
There was a problem hiding this comment.
Nope, this is just the name of the service. We need the topic to stay as ingest-feedback-events since that is the topic being published/consumed from in prod
There was a problem hiding this comment.
Ok so only change line 366 to feedback-events-consumer?
docker-compose.yml
Outdated
| @@ -363,6 +363,9 @@ services: | |||
| transactions-consumer: | |||
| <<: *sentry_defaults | |||
| command: run consumer ingest-transactions --consumer-group ingest-consumer | |||
| ingest-feedback-events: | |||
| <<: *sentry_defaults | |||
| command: run consumer ingest-feedback-events --consumer-group ingest-consumer | |||
There was a problem hiding this comment.
Should we use ingest-feedback-events as the consumer group instead?
There was a problem hiding this comment.
Does this consumer group exist for self-hosted? I was following the convention of the other ingest consumers in this file
There was a problem hiding this comment.
Will the command create the group? If so, sure I can change that
There was a problem hiding this comment.
Other ingest consumers in this file also have different consumer groups. The reason why transactions, attachments, and events all have the same consumer group is because they used to be combined, but were split in https://github.com/getsentry/self-hosted/pull/2193/files
aliu39
force-pushed
the
aliu/feedback-topic-and-consumer
branch
from
d54e6e7
to
60f9681
Compare
aliu39
force-pushed
the
aliu/feedback-topic-and-consumer
branch
from
60f9681
to
0c82dd7
Compare
|
My bad, lost the commit history reverting some stuff, I thought I was on another branch.. |
No, think we'll need #3061 for that. I'll merge and test it tomorrow! |
|
@aliu3ntry You should be able to test your changes out before merging |
|
Had trouble ingesting feedback in self-hosted instance due to CORS error / msg being dropped by proxy before it could reach the relay processor. Tried debugging and discussed with @hubertdeng123. It'd be nice to get this working so we can enable feedback by default. But due to other commitments, we'll come back to it later. |
I recently migrated feedback off the ingest-events pipeline in all prod regions: see getsentry/sentry#66100.
This isn't a pressing change for self-hosted, but without it, we can't clean up this option which we use like a FF in relay.
Needed for getsentry/relay#3604