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

chore(batch-exports): Add batch export monitoring workflow #26728

Merged
merged 8 commits into from
Dec 13, 2024

Conversation

rossgray
Copy link
Contributor

@rossgray rossgray commented Dec 6, 2024

Problem

In the past we've had some issues with events missing from certain batch exports and lack any reliable monitoring for this.

Changes

Add a new workflow in Temporal for monitoring the events exported by a given team's batch export.

This workflow is quite specific at the moment, in that it just checks 5 min batch exports (since these are the only ones using the recent_events table). It checks for a previous hour's worth of events (based on inserted_at) and stores the event count in the batch_export_runs table, so we can later reconcile this against records_completed.

I have left a few TODOs in the code for things I'm unsure about.

Does this work well for both Cloud and self-hosted?

it doesn't have an impact

How did you test this code?

Added tests

@posthog-bot
Copy link
Contributor

Hey @rossgray! 👋
This pull request seems to contain no description. Please add useful context, rationale, and/or any other information that will help make sense of this change now and in the distant Mars-based future.

@rossgray rossgray changed the title Add batch export monitoring workflow chore(batch-exports): Add batch export monitoring workflow Dec 6, 2024
@rossgray rossgray requested a review from tomasfarias December 6, 2024 14:15
Copy link
Contributor

@tomasfarias tomasfarias left a comment

Choose a reason for hiding this comment

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

Solution and implementation look really good. I have a handful of comments to address scattered all over, but nothing critical.

@rossgray rossgray force-pushed the ross/batch-exports-alerts branch from 4fd7d97 to 5f2e8ed Compare December 10, 2024 14:53
@rossgray
Copy link
Contributor Author

@tomasfarias thanks for the review! Think I've addressed all your feedback. Going to spend some more time investigating what's going on with this test

Copy link
Contributor

@tomasfarias tomasfarias left a comment

Choose a reason for hiding this comment

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

I think PR looks great. Once tests are passing this is good to merge! Good work!

@rossgray rossgray force-pushed the ross/batch-exports-alerts branch from 5f2e8ed to 70f3758 Compare December 11, 2024 16:56
@rossgray rossgray force-pushed the ross/batch-exports-alerts branch from 70f3758 to 456bc73 Compare December 12, 2024 14:12
@rossgray rossgray merged commit 7f06bec into master Dec 13, 2024
89 checks passed
@rossgray rossgray deleted the ross/batch-exports-alerts branch December 13, 2024 09:47
Copy link

sentry-io bot commented Dec 13, 2024

Suspect Issues

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

  • ‼️ ActivityError: Activity task failed posthog.temporal.batch_exports.monitoring in run View Issue
  • ‼️ NoValidBatchExportsFoundError: Only batch exports with interval of 5 minutes are supported for monitoring at this time. posthog.temporal.batch_exports.monitoring in ge... View Issue
  • ‼️ OperationalError: consuming input failed: query_wait_timeout posthog.batch_exports.service in aupdate_record... 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.

3 participants