Skip to content
This repository has been archived by the owner on Nov 4, 2021. It is now read-only.

Rework console extension to send data to Kafka>ClickHouse or Postgres #318

Merged
merged 14 commits into from
Apr 22, 2021

Conversation

Twixes
Copy link
Collaborator

@Twixes Twixes commented Apr 15, 2021

Changes

Plugin server changes for PostHog/posthog#3482.

Checklist

  • Jest tests

Copy link
Contributor

@yakkomajuri yakkomajuri left a comment

Choose a reason for hiding this comment

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

Sincerely naive question, but how come not just use Postgres for this given we still store data there for both EE and FOSS? Is it a matter of future-proofing and keeping as much of the new additions specific to one db?

src/worker/vm/extensions/console.ts Outdated Show resolved Hide resolved
@Twixes
Copy link
Collaborator Author

Twixes commented Apr 16, 2021

Given the potentially high data volume nature of this feature, ClickHouse is definitely more future-proof and only marginally more complicated. @yakkomajuri

@yakkomajuri
Copy link
Contributor

Tests are still expecting stuff like console.log to be called. Even though we're piping logs somewhere else, we could still call the native console methods, no?

Either way, if the answer is no, I guess tests still have to be updated

@Twixes Twixes added bump minor Bump minor version when this PR gets merged bump patch Bump patch version when this PR gets merged and removed bump minor Bump minor version when this PR gets merged labels Apr 20, 2021
@Twixes
Copy link
Collaborator Author

Twixes commented Apr 20, 2021

Yep, adjusted for that @yakkomajuri. FYI in-plugin console method calls are now status'd to stdout in dev mode (DEBUG truthy or NODE_ENV is dev).
👉

@Twixes
Copy link
Collaborator Author

Twixes commented Apr 20, 2021

PR reviewable.
This functionality (including its tests) is completely disabled by default currently though, configurable with setting ENABLE_PERSISTENT_CONSOLE. That's because we should first merge this to make PostHog/posthog#3482 work end-to-end, then merge PostHog/posthog#3482, and then remove ENABLE_PERSISTENT_CONSOLE when the functionality can be put into production (mind that PostHog Cloud will require a migration to be manually initiated for this to work, Infra teams knows about this).

Checks

Top green check is the end goal state - persistent console fully tested and enabled by default. That ran with PostHog/posthog#3482 as the main repo base though, so until that is merged into main repo master, the version merged will have some if (!server.ENABLE_PERSISTENT_CONSOLE) return conditions – bottom green check.

src/shared/db.ts Outdated Show resolved Hide resolved
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.

LGTM. Merge whenever convenient.

@Twixes Twixes removed the request for review from yakkomajuri April 22, 2021 12:23
@Twixes Twixes merged commit 22db5c7 into master Apr 22, 2021
@Twixes Twixes deleted the plugin-logs branch April 22, 2021 12:23
fuziontech pushed a commit to PostHog/posthog that referenced this pull request Oct 12, 2021
…ouse or Postgres (PostHog/plugin-server#318)

* Rework console extension to send data to Kafka>ClickHouse or Postgres

* Fix object stringification in console

* Log ConsoleExtension to stdout in dev

* Hide rework behind ENABLE_PERSISTENT_CONSOLE setting

* Add tests

* Run CI in respect to main repo plugin-log-entries branch

* Update sql.ts

* Fix tests

* Update plugins.test.ts

* Don't run console tests if ENABLE_PERSISTENT_CONSOLE false

* Use plugin_config_id in plugin log entry

* Improve persistent console value formatting

* Do run console tests if ENABLE_PERSISTENT_CONSOLE false

* Revert "Do run console tests if ENABLE_PERSISTENT_CONSOLE false"

This reverts commit 2e5f652b39e65eb2e612de38dc4cec21baa6842b.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bump patch Bump patch version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants