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: Destination tests for batch exports #28687

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

tomasfarias
Copy link
Contributor

@tomasfarias tomasfarias commented Feb 13, 2025

Problem

The most common type error when interacting with batch exports are user configuration errors. This PR introduces a new framework for "Destination tests": Tests that can be called from our API to evaluate a given batch export configuration.

Changes

  • [x]: Bigquery destination tests
  • [ ]: Snowflake destination tests
  • [x]: S3 destination tests
  • [ ]: PostgreSQL destination tests
  • [ ]: Redshift destination tests
  • [x]: Frontend support
  • [x]: API Support

Frontend screenshots:

All passed:
image

Error:
image

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

New unit tests.

@tomasfarias tomasfarias force-pushed the feat/destination-tests-for-batch-exports branch from 43f589a to 2dab705 Compare February 14, 2025 10:04
Comment on lines 529 to 530
@action(methods=["POST"], detail=True)
def test(self, request: request.Request, *args, **kwargs) -> response.Response:
Copy link
Contributor

Choose a reason for hiding this comment

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

is this endpoint just for fetching a test (and it's list of steps) for a given batch export?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, thought about having everything in a single endpoint, but it looked too complicated. I'd be happy to rename this to something more verbose though...

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it does make sense to get a list of possible tests upfront, especially if we want to add them to destinations one at a time, since the frontend could probably not even show a test button if there are no tests to run for example. Was also wondering if it could just be a GET method if it's not actually running anything

@tomasfarias tomasfarias marked this pull request as ready for review February 18, 2025 15:51
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

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here: app.greptile.com/review/github.

10 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

@tomasfarias tomasfarias force-pushed the feat/destination-tests-for-batch-exports branch from b26a321 to 752805b Compare February 19, 2025 12:29
Copy link
Contributor

github-actions bot commented Feb 19, 2025

Size Change: +220 B (0%)

Total Size: 9.7 MB

ℹ️ View Unchanged
Filename Size Change
frontend/dist/toolbar.js 9.7 MB +220 B (0%)

compressed-size-action

@tomasfarias tomasfarias requested a review from rossgray February 19, 2025 16:20
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