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: Switch batch exports logging to async #24819

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

tomasfarias
Copy link
Contributor

@tomasfarias tomasfarias commented Sep 5, 2024

Problem

Had good results with moving file writing to a thread in #24792. Inspired by that, I reviewed where we could have more blocking code, and that lead me to our logging (and also to #24804).

Structlog provides async methods that use a thread pool for logging. Our processor chain is not that complex that should be blocking for too long, but occasional timeouts tell us that we still have some blocking code. As little as it may be, Temporal workflows work best when nothing is blocking, so, let's also not block when logging. Our workflows are entirely bound by IO, so the extra computational overhead is not significant for our use case.

Changes

  • Switch all batch export logging calls to async variants (prefixed with a).

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

Nothing really changed, functionality-wise. So, existing tests should cover this PR.

@tomasfarias tomasfarias requested a review from a team September 5, 2024 16:57
@tomasfarias tomasfarias merged commit db12814 into master Sep 6, 2024
93 checks passed
@tomasfarias tomasfarias deleted the feat/switch-batch-exports-logging-to-async branch September 6, 2024 07:52
Copy link

sentry-io bot commented Sep 8, 2024

Suspect Issues

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

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