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: Implement structlog batch exports logger #18334

Closed
wants to merge 23 commits into from

Conversation

tomasfarias
Copy link
Contributor

@tomasfarias tomasfarias commented Nov 2, 2023

Problem

The existing logging implementation for batch exports is a bit clunky:

  • We hardcode the log source to "batch_exports", but it would be nice to distinguish from backfills.
  • Logs are not rendered as JSON, so they would not be parse-able by log aggregators (Loki).
  • Logs repeat themselves: Every log line says the export type.
  • All the code is in the generic batch_exports.py module.
  • Kafka logging handler is threaded instead of async. Having an async handler would allow us to not block other tasks (mostly heartbeats).
  • We rely on global variables (yuck).

Changes

  • Move over to structlog to handle logging (structlog is already a dependency from the django-structlog requirement, so nothing new here, but I did bump the version, which should be supported). This allows us to set context variables (like team_id and destination) once and for all.
  • Moreover, structlog allows us to render logs as JSON, so log aggregators will be happy.
  • Move logger to its own module.
  • Use aiokafka to make kafka producer async.

Overall, the logging system now looks easier to understand and, eventually, extend. The line diff may be deceptive as a lot of it is in docstrings to aid with understanding and more than half is in unit testing (which we didn't properly have much of).

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

How did you test this code?

Added unit tests.

Base automatically changed from refactor/abstract-insert-activty-for-batch-exports to master November 2, 2023 09:38
@tomasfarias tomasfarias force-pushed the refactor/batch-exports-logging branch 3 times, most recently from 4608347 to ed57cc9 Compare November 2, 2023 22:11
@tomasfarias tomasfarias marked this pull request as ready for review November 2, 2023 22:39
@tomasfarias tomasfarias marked this pull request as draft November 3, 2023 17:40
@tomasfarias
Copy link
Contributor Author

Going to keep this one in draft until we merge #18380 and we can rebase on that before addressing test failures.

@tomasfarias tomasfarias changed the title feat: Implement new structlog batch exports logger feat: Implement structlog batch exports logger Nov 6, 2023
@tomasfarias tomasfarias force-pushed the refactor/batch-exports-logging branch from 0206f1b to 2182351 Compare November 7, 2023 13:59
@tomasfarias tomasfarias force-pushed the refactor/batch-exports-logging branch from 6920e68 to 7f371cd Compare November 7, 2023 15:42
@tomasfarias
Copy link
Contributor Author

Closed in favor of: #18458

@tomasfarias tomasfarias closed this Nov 7, 2023
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.

1 participant