Skip to content

chore: change guidance on starting application #27

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

davydog187
Copy link
Contributor

Hello, thank you for the Elixir client library for Posthog! I am evaluating Posthog for my business, TV Labs, and after doing due-diligence, I wanted to send a few improvements. (This is the first)

Since Elixir 1.4 (January, 2017), Elixir automatically starts all Applications in your dependencies. Adding :posthog to your :extra_applications is thus unnecessary, and probably bad advice for new-comers to the language.

Starting it as part of your application supervision tree is also probably misguided.

As an aside, if this library wanted to give users more control over supervising their instance, it should not start an application, and instead expose a root GenServer where all configuration is passed.

Since Elixir 1.4 (January, 2017), Elixir automatically starts all
Applications in your dependencies. Adding `:posthog` to your
`:extra_applications` is thus unnecessary, and probably bad advice for
new-comers to the language.

Starting it as part of your application supervision tree is also
probably misguided.

As an aside, if this library wanted to give users more control over
supervising their instance, it should not start an application, and
instead expose a root GenServer where all configuration is passed.
@rafaeelaudibert
Copy link
Member

Hey, @davydog187! More than happy to receive contributions in here. I've used Elixir in the past but not in the same capacity as you're using, so I'm all ears to your recommendations.

How would you suggest we implement this generic GenServer? Right now, we only need an application because we're packaging Cachex to help with feature flag evaluation.

Eventually, however, we'll want to build a local-evaluation implementation for Elixir, which will most definitely use a GenServer and some :ets tables. Any suggestions on the best way to implement Cachex now + allow us to easily expand to a :ets genserver in the future?

As for this PR, although I agree we don't need people to include them in their :extra_applications list, do you believe it's a bad suggestion to let people know that they can customize the application if they want to? Some people might wanna run them under a different default in their supervision tree, correct?

@davydog187
Copy link
Contributor Author

davydog187 commented Apr 24, 2025

Thanks for the quick response @rafaeelaudibert. Let me respond to each comment

Right now, we only need an application because we're packaging Cachex to help with feature flag evaluation.

There's two reasons why you're using Applications as far as I can tell.

  1. Application config
  2. Automatically starting resources for Posthog such as Cachex

Having Posthog as an application is fine, and probably the simplest way. By putting Cachex in your application supervisor, clients don't need to worry about making sure your dependencies are up and running, it just happens automatically.

How would you suggest we implement this generic GenServer?

The reason why you might want to do something like this is if there is a reason for a user to run multiple different configurations of Posthog in one project. I'm not sure if this is relevant to any use cases you can think of, but it would mean making Posthog or some other module a Supervisor, and then passing the configuration to it directly, rather than relying on Application configuration. Users could then use the configuration of their own application to drive the API key and any other global configuration, e.g.

defmodule MyApp.Application do
  use Application
  
  def start(_type, _args) do
    children = [
      {Posthog, Application.get_env(:my_app, :posthog_configuration_one)},
      {Posthog, Application.get_env(:my_app, :posthog_configuration_two)},
      MyAppWeb.Endpoint
    ]

    opts = [strategy: :one_for_one, name: MyApp.Supervisor]
    Supervisor.start_link(children, opts)
  end
end

This gives users more control over how the app is configured and started.

Eventually, however, we'll want to build a local-evaluation implementation for Elixir, which will most definitely use a GenServer and some :ets tables. Any suggestions on the best way to implement Cachex now + allow us to easily expand to a :ets genserver in the future?

For HOW to do it, you would make Posthog be a supervisor, and then any ets tables or anything named should be generated generated off the root name, e.g. if the the name of the first was :posthog_a, then you would generate an ets tabel name like :posthog_a_ets_cache

do you believe it's a bad suggestion to let people know that they can customize the application if they want to? Some people might wanna run them under a different default in their supervision tree, correct?

Possibly! However, this isn't unique to Posthog and is advanced usage. If you need this, its the same for all libraries, and you probably know what you're doing already.

Hope that helps! Happy to jump on a call if you want to discuss further

@davydog187
Copy link
Contributor Author

davydog187 commented Apr 25, 2025

@rafaeelaudibert I want to clarify one more thing.

The guidance suggesting users put Posthog.Application in their application supervision tree may be misguided, as it's likely already started. Users would need to explicitly specify their applications, remove Posthog, and then add it back into their application.

This is why I'm suggesting that if you want to enable that level of flexibility, a better approach would be to move away from Posthog being an application at all. However, one thing I didn't mention in my previous replies is that if you take this route, any client functions such asPosthog.feature_flag/3 would need to include an argument specifying which cache they should communicate with.

@rafaeelaudibert
Copy link
Member

@davydog187 Ok, that all makes a lot of sense, thank you for being so careful.

I'll merge this PR as is because I don't think we'll want people to have to list all their applications just to customize ours. Similarly, adding an extra cache parameter to the FeatureFlag calls is very unusual.

Maybe we can let people pass a Cachex process in via the config.exs file? Is that reasonable/common in the Elixir world?

@davydog187
Copy link
Contributor Author

davydog187 commented Apr 29, 2025

I'll merge this PR as is because I don't think we'll want people to have to list all their applications just to customize ours. Similarly, adding an extra cache parameter to the FeatureFlag calls is very unusual.

Given the explicit nature of Elixir, I don't think its that unusual, but I don't think its the best API design

Maybe we can let people pass a Cachex process in via the config.exs file? Is that reasonable/common in the Elixir world?

I think this needs to be done from the caller, however, there might be a better way to do this that is both explicit and solves the problem. This is a common pattern in other libraries, but you could have something like this

defmodule MyApp.Posthog do
    use Posthog, name: :posthog_a, ... # some other compile-time configuration
    
    @impl Posthog
    def config do
      Application.get_env(:my_app, :posthog)
    end

   # the use Posthog injects the API of Posthog into this module, but with everything pre-configured
 end
 
 # Then the user starts MyApp.Posthog in their application supervision tree rather than Posthog directly
 
 defmodule MyApp.Application do
   use Application
  
  def start(_type, _args) do
    children = [
      MyApp.Posthog,
      MyApp.SomeOtherPosthog,
      MyAppWeb.Endpoint
    ]

    opts = [strategy: :one_for_one, name: MyApp.Supervisor]
    Supervisor.start_link(children, opts)
  end
end

# then in your app, you would use functions on your own MyApp.Posthog module, which delegate with the right config to the Posthog module

MyApp.Posthog.feature_flag("foo", "123")

I think this would be a really easy transition and would give you a lot more flexibility and opportunities to keep the API simple for the user. The implementation would be really easy too. If you're interested in this direction I'd be happy to help or even send a PRi

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.

2 participants