Skip to content

ref(devserver): Overhaul interplay between kafka consumers and --workers#51664

Merged
untitaker merged 3 commits intomasterfrom
ref/devserver-logic
Jun 28, 2023
Merged

ref(devserver): Overhaul interplay between kafka consumers and --workers#51664
untitaker merged 3 commits intomasterfrom
ref/devserver-logic

Conversation

@untitaker
Copy link
Copy Markdown
Member

@untitaker untitaker commented Jun 26, 2023

Some of our kafka consumers implicitly started when --workers was not
provided, and some of them started even though kafka itself was disabled
in sentry.conf.py. This causes great confusion because in the case of
getsentry outcomes consumer, we started a consumer without ensuring kafka
was running.

This new setup removes a ton of duplicated sanity-checks around kafka,
and replaces it with one flow:

  • --workers is required to start ANY Kafka consumer (breaking change)
  • --ingest requires --workers, because without --workers there's no
    ingest consumer (breaking change)
  • Relay is not started without --workers (plain devserver literally only
    starts webpack and uwsgi no matter what) (also breaking change)
  • Random product feature flags append to kafka_consumers list
  • If that list ends up being non-empty but we find no kafka/zookeeper
    containers, we tell the user to fix it (use kafka eventstream or
    SENTRY_USE_RELAY)

Some of our kafka consumers implicitly started when --workers was not
provided, and some of them started even though kafka itself was disabled
in sentry.conf.py

This new setup removes a ton of duplicated sanity-checks around kafka,
and replaces it with one flow:

* --workers is required to start ANY Kafka consumer (breaking change)
* --ingest requires --workers, because without --workers there's no
  ingest consumer (breaking change)
* Relay is not started without --workers (plain devserver literally only
  starts webpack and uwsgi no matter what) (also breaking change)
* Random product feature flags append to kafka_consumers list
* If that list ends up being non-empty but we find no kafka/zookeeper
  containers, we tell the user to fix it (use kafka eventstream or
  SENTRY_USE_RELAY)
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jun 26, 2023
@untitaker untitaker requested review from a team June 26, 2023 22:32
@untitaker
Copy link
Copy Markdown
Member Author

tagging some random teams that I think deal most with kafka consumers in sentry

@untitaker untitaker requested a review from a team June 26, 2023 22:33
@untitaker untitaker changed the title ref: Overhaul interplay between kafka consumers and --workers ref(devserver): Overhaul interplay between kafka consumers and --workers Jun 26, 2023
@codecov
Copy link
Copy Markdown

codecov bot commented Jun 26, 2023

Codecov Report

Merging #51664 (99f23b1) into master (1aa03d1) will increase coverage by 1.51%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff            @@
##           master   #51664     +/-   ##
=========================================
+ Coverage    79.80    81.31   +1.51     
=========================================
  Files        4873     4899     +26     
  Lines      205257   205744    +487     
  Branches    11068    11068             
=========================================
+ Hits       163797   167287   +3490     
+ Misses      41215    38212   -3003     
  Partials      245      245             

see 352 files with indirect coverage changes

@markstory
Copy link
Copy Markdown
Member

--ingest requires --workers, because without --workers there's no
ingest consumer (breaking change)

Should this be implicit? Having just --ingest isn't that useful as you'll be able to accept events but not have them show up anywhere. We could have

  • --ingest enables celery workers, kafka consumer + relay. Gives you a functioning ingestion pipeline locally.
  • --workers runs celery workers and kafka consumers. Not all of the consumers will be doing work but that's ok.

@untitaker
Copy link
Copy Markdown
Member Author

Should this be implicit? Having just --ingest isn't that useful as you'll be able to accept events but not have them show up anywhere. We could have

this PR makes devserver explicitly reject --ingest without --workers. would you prefer it to implicitly enable --workers?

@markstory
Copy link
Copy Markdown
Member

would you prefer it to implicitly enable --workers?

Yeah, Instead of having to write out both options, --ingest starts the processes that --workers would as well as the rest of the ingestion pipeline.

@untitaker
Copy link
Copy Markdown
Member Author

Done!

Copy link
Copy Markdown
Member

@markstory markstory left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@untitaker untitaker merged commit ac09fba into master Jun 28, 2023
@untitaker untitaker deleted the ref/devserver-logic branch June 28, 2023 14:42
@github-actions github-actions bot locked and limited conversation to collaborators Jul 14, 2023
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