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

More readme context and cleanup - #389

Merged
Twixes merged 3 commits into
masterfrom
readme-update
May 19, 2021
Merged

More readme context and cleanup#389
Twixes merged 3 commits into
masterfrom
readme-update

Conversation

@mariusandra

Copy link
Copy Markdown
Collaborator

Changes

@neilkakkar it was really awesome to see the written breakdown of the plugin server. Thanks for this.

I made a few quick tweaks and added a bit of extra context on a few points, however I feel like there's still a lot that could be added.

My only worry with this walkthrough is that the plugin server code changes so rapidly, that we must make a targeted effort to keep it up to date, and remember to do so in every PR. I'm not sure if we'll be able to keep up the discipline. We can try.

However as general feedback, some parts of the walkthrough are very high level, describing how the different parts of the system interact, yet some paragraphs go very deep into the code, such as:

"This createWorker() is a function returning createTaskRunner, which is a curried function, which given {task, args}, returns workerTasks[task](server, args).".

I think we should remain strictly high level, as I definitely will not think of updating a readme when I change such implementation details :).

@Twixes any thoughts or context to add to this section of the readme?

Checklist

  • Updated Settings section in README.md, if settings are affected
  • Jest tests

@mariusandra
mariusandra requested a review from Twixes May 18, 2021 11:59
@neilkakkar

Copy link
Copy Markdown
Contributor

Fair points!

Thanks for taking the time to walk me through the basics, that made this process so much faster :)

I worry about it going out of date as well, but I think outdated info is better than none - it atleast helps us "not start from scratch" when we want to add something.

I'm working towards adding the rest of the sections, slowwwly.

@Twixes

Twixes commented May 18, 2021

Copy link
Copy Markdown
Member

Really cool to have this! And sure, no rush @neilkakkar, first soak the system in. ;)

A few points from me:

  • The last main thread paragraph (on queues) is a bit confusing. For instance point "queue: Wait, another queue?" is not informative :D The info is there, it can just be distilled better.
  • One pluginsServer component is missing: mmdbServer (and related mechanisms) which does quite a bit to allow scalable GeoIP usage.
  • "team_id represents a company using plugins, and each team can have its own set of plugins enabled" is not exactly right. An organization_id is tied to a company and its installed plugins, a team_id is tied to a project and its plugin configs (enabled/disabled+extra config)

As for the "Where is team_id populated?" question, that value in event processing comes from the main PostHog repo (log_event Python function), where the data is passed to this service via Kafka. :)

And definitely agree on not singling out curried functions, details like that.

@Twixes

Twixes commented May 19, 2021

Copy link
Copy Markdown
Member

Made some changes, I think this is a bit more polished now, though still definitely not the definitive version.

@Twixes
Twixes merged commit 2a07591 into master May 19, 2021
@Twixes
Twixes deleted the readme-update branch May 19, 2021 13:34
fuziontech pushed a commit to PostHog/posthog that referenced this pull request Oct 12, 2021
…r#389)

* some more context and cleanup

* Reword Walkthrough a bit and add mmdbServer + plugins/plugin configs

Co-authored-by: Michael Matloka <dev@twixes.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants