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: Hog transformation comparer #28104

Merged
merged 19 commits into from
Jan 31, 2025
Merged

Conversation

benjackwhite
Copy link
Contributor

@benjackwhite benjackwhite commented Jan 30, 2025

Problem

We want to safely roll out the migration to hog functions but the question is - how?

Changes

  • Adds a comparer step that compares the plugin transforms to the hog version
  • Adds a migration script to migrate all plugin ones over to hog ones
  • Adds an env var for sampling this so we don't hit the ingestion pipeline too hard
  • Fixes the migrator to work for transformations as well
  • Fixes the UI so that we have a way of saying an input doesn't support templating (so that hog bytecode isn't generated)

👉 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?

Copy link
Contributor

github-actions bot commented Jan 31, 2025

Size Change: -5 B (0%)

Total Size: 1.16 MB

ℹ️ View Unchanged
Filename Size Change
frontend/dist/toolbar.js 1.16 MB -5 B (0%)

compressed-size-action

@benjackwhite benjackwhite marked this pull request as ready for review January 31, 2025 10:33
@benjackwhite benjackwhite requested a review from a team January 31, 2025 10:35
@@ -164,20 +168,38 @@ export class HogTransformerService {
return promises
}

public transformEvent(event: PluginEvent): Promise<TransformationResult> {
public transformEventAndProduceMessages(event: PluginEvent): Promise<TransformationResult> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wasn't sure of the naming here but basically wanted one function that does the transform and another that does it and produces the kafka messages

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR introduces a comprehensive system for safely migrating from plugin transformations to hog functions. Here's a summary of the key changes:

  • Added comparison mechanism in compareToHogTransformStep.ts to validate hog transformations against existing plugin outputs with configurable sampling rate
  • Introduced templating: false property across transformation plugin templates to explicitly control template processing behavior
  • Modified HogTransformerService to separate message production from event transformation logic and improve error handling
  • Added migration scripts with --kind parameter to support selective migration of 'destination' or 'transformation' plugins
  • Added HOG_TRANSFORMATIONS_COMPARISON_PERCENTAGE config to control sampling rate and prevent excessive load on ingestion pipeline

Key points to review:

  • The hardcoded project name in createInvocationGlobals needs to be addressed
  • The execute method in HogTransformerService doesn't await hogExecutor.execute result which could cause timing issues
  • The downsampling plugin template has a simplified return event implementation that may not fully replicate original functionality
  • Error handling in transformEvent continues execution after errors instead of failing fast

22 file(s) reviewed, 13 comment(s)
Edit PR Review Bot Settings | Greptile

Copy link
Contributor

@pl pl left a comment

Choose a reason for hiding this comment

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

Looked at the TS code, LGTM. I haven't worked on the migration bit at all, so it's hard for me to do a review yet.

@benjackwhite benjackwhite merged commit 48f5a62 into master Jan 31, 2025
98 of 99 checks passed
@benjackwhite benjackwhite deleted the feat/hog-transformation-comparer branch January 31, 2025 14:38
thmsobrmlr pushed a commit that referenced this pull request Feb 3, 2025
Copy link

sentry-io bot commented Feb 4, 2025

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ ValidationError: {"template_id":["Transformation functions must be created from a template."]} posthog.cdp.migrations in migrate_legacy_plugins View Issue
  • ‼️ TypeError: Cannot create distinct fields once a slice has been taken. posthog.cdp.migrations in migrate_batch View Issue
  • ‼️ ValidationError: {"template_id":["Transformation functions must be created from a template."]} posthog.cdp.migrations in migrate_batch View Issue
  • ‼️ KeyError: 117775 posthog.cdp.migrations in migrate_batch View Issue
  • ‼️ KeyboardInterrupt posthog.hogql.ast in __init__ View Issue

Did you find this useful? React with a 👍 or 👎

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.

2 participants