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

Enforce (or provide user-facing warnings) instantiating the Python client as a singleton #152

Open
dmarticus opened this issue Dec 17, 2024 · 4 comments

Comments

@dmarticus
Copy link
Contributor

Full context can be found here: https://posthog.slack.com/archives/C03P7NL6RMW/p1733514026622539, but here's the summary for external stakeholders:

Issue:

  • Users were experiencing dropped events when using the Python SDK
  • Feature flag events were being captured but other events weren't
  • Investigation revealed multiple SDK client instances were being instantiated
  • No warnings/exceptions were thrown on multiple client instantiations

Root Cause:

  • The SDK doesn't enforce a singleton pattern for client instances
  • Multiple client instances in the same application can cause event capture issues
  • This becomes particularly problematic in containerized environments like Kubernetes

Solution:

  • Implement a singleton factory pattern to prevent multiple client instantiations
  • Add explicit warnings/exceptions when users try to create multiple clients
  • Update documentation to:
    • Clearly explain the singleton requirement
    • Provide best practices for client instantiation
    • Include guidance for complex infrastructure setups
@haacked
Copy link
Contributor

haacked commented Mar 1, 2025

@dmarticus just curious, it looks like you had a promising PR to this here: #151
Was there something fundamentally wrong with that approach, or did other priorities take precedence? Since this is on my onboarding checklist, I can carry it to completion if you'd like me to take over.

@dmarticus
Copy link
Contributor Author

@dmarticus just curious, it looks like you had a promising PR to this here: #151 Was there something fundamentally wrong with that approach, or did other priorities take precedence? Since this is on my onboarding checklist, I can carry it to completion if you'd like me to take over.

Yeah, nothing fundamentally wrong, I just never finished it and then other priorities took over (this was a weekend hack job so it's not principled, even if the ideas were good). You're more than welcome to take it over and get it over the line :)

@neilkakkar
Copy link
Contributor

I don't have full context sorry, so not sure if this is correct, buuut we already do have both modes of operation. If you want to enforce a singleton-like behaviour - you can use the non-instantiation method of using the client (see posthoganalytics in our main app - this is what we do for the main app, and I think setup new client instances for celery when we need them, since it's effectively a different application). Maybe what we're missing here are the docs to do so, since we removed those (sort of) and recommended the instantiation method for the client, because of one big problem with the singleton-like method: If you're using this, and you're also using an external lib that's also using the singleton-like module based posthog client, these clients clash and stop sending events to one, overshadowed and send all events to the latest imported client.

It's unclear to me how / when this issue occurs unless users are explicitly creating new clients? A bit reluctant to have yet another way to do things, specially when we don't really want to deprecate the current method (contra existing PR), because there are valid cases where you want to send different events to different instances, and the only good way to do that is to have separate clients.

@lricoy
Copy link
Member

lricoy commented Mar 28, 2025

FYI, this was brought to discussion again today due to this ticket investigation: https://posthog.slack.com/archives/C0864Q9802Y/p1743098163480279

It's not exactly the same as the original thread, but the solution was to use sync_mode=True as well, so I'm +1 on more docs/warnings if we can.

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

No branches or pull requests

4 participants