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

Plugin log entries #3482

Merged
merged 61 commits into from
May 6, 2021
Merged

Plugin log entries #3482

merged 61 commits into from
May 6, 2021

Conversation

Twixes
Copy link
Collaborator

@Twixes Twixes commented Feb 25, 2021

Changes

This is a proposed the accepted and awaiting complete implementation in this PR (and a complimentary one in https://github.com/PostHog/plugin-server) data model to resolve PostHog/plugin-server#72.
The unit of plugin logging is a single plugin log entry (PluginLogEntry model for Postgres and a schema-wise drop-in equivalent plugin_log_entries topic → kafka_plugin_log_entries table → plugin_log_entries table setup in Kafka+ClickHouse).

This looks like some complexity at first, but it's straightforward compared to fighting Postgres's scale limitations:

  • For FOSS users, the solution is the simplest possible plain INSERTs/SELECTs on posthog_pluginlogentry, and we know they won't have problems with scale.
  • For Cloud/EE, we need to support LARGE scale with minimum maintenance and high data integrity. For this part Kafka+ClickHouse is unbeatable within our existing infrastructure. And also very simple "produce to topic on every console method call in plugin server, SELECT relevant entries from table in Django server".

Kafka+ClickHouse parts adapted from session recording events handling, which is similarly simple (though actually logs are far better for this infrastructure than session recording, which is too storage space-intensive).

@timgl timgl temporarily deployed to posthog-plugin-log-entr-51oxqz February 25, 2021 13:53 Inactive
@Twixes Twixes temporarily deployed to posthog-plugin-log-entr-51oxqz February 25, 2021 13:54 Inactive
Copy link
Collaborator

@mariusandra mariusandra 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 we can go in this direction. The only thing we'll definitely need to add then is some decent TTL restrictions in clickhouse (30 days?) and a script to delete old rows from postgres, like we have now for session recordings. The size of this table should probably also be added to the system status page.

posthog/models/plugin.py Outdated Show resolved Hide resolved
posthog/models/plugin.py Show resolved Hide resolved
@mariusandra
Copy link
Collaborator

Also, into the models, I'd add another field for "instance_id" - a random ID per rebooted plugin server. This will help read the logs if there are multiple plugins talking at the same time.

@Twixes Twixes temporarily deployed to posthog-plugin-log-entr-51oxqz March 2, 2021 11:05 Inactive
@Twixes Twixes temporarily deployed to posthog-plugin-log-entr-51oxqz March 2, 2021 11:06 Inactive
@Twixes Twixes temporarily deployed to posthog-plugin-log-entr-51oxqz March 2, 2021 11:36 Inactive
@Twixes
Copy link
Collaborator Author

Twixes commented Mar 2, 2021

Great, I'll add the rest of this solution after putting #3486 up for review.

@Twixes Twixes temporarily deployed to posthog-pr-3482 April 26, 2021 12:54 Inactive
@Twixes Twixes temporarily deployed to posthog-pr-3482 April 26, 2021 13:17 Inactive
@Twixes
Copy link
Collaborator Author

Twixes commented Apr 26, 2021

Added support for system (plugin server) logs too (PostHog/plugin-server#333):

1
2

@Twixes Twixes temporarily deployed to posthog-pr-3482 April 26, 2021 14:05 Inactive
@Twixes Twixes temporarily deployed to posthog-pr-3482 April 26, 2021 14:06 Inactive
@Twixes Twixes temporarily deployed to posthog-pr-3482 April 26, 2021 14:07 Inactive
@Twixes
Copy link
Collaborator Author

Twixes commented Apr 26, 2021

This is ready. @macobo After the migration is ran, the logs won't be produced yet – they have to be enabled with env var ENABLE_PERSISTENT_CONSOLE in the plugins task def. After that we'll be able to remove that env var and enable this feature by default in another PR.

@Twixes Twixes temporarily deployed to posthog-pr-3482 April 26, 2021 17:57 Inactive
@Twixes Twixes temporarily deployed to posthog-pr-3482 April 26, 2021 18:22 Inactive
@Twixes Twixes temporarily deployed to posthog-pr-3482 April 26, 2021 19:03 Inactive
@Twixes Twixes temporarily deployed to posthog-pr-3482 April 26, 2021 19:43 Inactive
@mariusandra
Copy link
Collaborator

mariusandra commented Apr 29, 2021

@macobo @fuziontech can we do anything from our side to help move this along? Doing things manually once would also be fine perhaps.... though, yeah, VPCs...

@macobo
Copy link
Contributor

macobo commented Apr 29, 2021

Sorry for the delays, have had my own fires to fight. Will get this in soon! :)

@Twixes Twixes temporarily deployed to posthog-pr-3482 April 30, 2021 14:32 Inactive
@macobo macobo merged commit e96f95e into master May 6, 2021
@macobo macobo deleted the plugin-log-entries branch May 6, 2021 07:54
@Twixes Twixes added the highlight ⭐ Release highlight label May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
highlight ⭐ Release highlight
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stream console.log output somewhere
5 participants