-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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(cdp): Mini purge of old ingesters #29341
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
Mini purge of old ingesters
This PR removes deprecated ingestion systems from the plugin server, focusing on cleaning up old ingestion modes and their corresponding files that are no longer in use.
- Removed several
PluginServerMode
entries includingingestion
,ingestion_overflow
,ingestion_historical
,events_ingestion
, andanalytics_ingestion
- Deleted four ingestion consumer files:
analytics-events-ingestion-consumer.ts
,analytics-events-ingestion-overflow-consumer.ts
,analytics-events-ingestion-historical-consumer.ts
, andevents-ingestion-consumer.ts
- Updated
PROCESS_EVENT_CAPABILITIES
inworker/vm/capabilities.ts
to only includeingestionV2
andingestionV2Combined
- Modified all tests to use
ingestionV2
instead of the deprecated ingestion capabilities - Removed corresponding test files for the deleted ingestion consumers
16 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests have all been replaced by the ingestion consumer tests with snapshots confirming things working as expected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left two very minor comments, LGTM.
status.info('🔄', `Running processEvent for team ${teamId}, ${event.event}, ${pluginMethodsToRun.length}`, { | ||
event, | ||
pluginMethodsToRun, | ||
}) // TODO: Remove this line |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: remove it now or later?
@@ -320,58 +314,6 @@ export async function startPluginsServer( | |||
} | |||
|
|||
// Below are all legacy consumers that will be replaced by the new ingestion consumer that covers all cases |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: this comment should also be removed as we're moving to the new ingestion consumer in this PR, right?
Problem
We're no longer using the old ingesters. Time to remove them and make sure all tests are pointing at new stuff
Changes
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Does this work well for both Cloud and self-hosted?
How did you test this code?