Introduce feature flags higher level functions #47
Conversation
This will make it more ergonomic for future updates. Most of the code stayed the same.
I've also added a helper `checked!` function that avoids the whole `{:ok, _}` check, but it's more prone to errors. People should still use `check` most of the time, but `check!` is more ergonomic for scripts
|
@martosaur I've added some commits here, what do you think? This feels more ergonomic and I like the new namespace to separate feature flag calls. How does it look for you? I've also got a new PR (tagging you soon) building on top of this one to add a new |
lib/posthog/feature_flags.ex
Outdated
| @doc false | ||
| @spec log_feature_flag_usage(supervisor_name(), String.t(), String.t(), {:ok, boolean() | String.t()} | {:error, Exception.t()}) :: :ok |
There was a problem hiding this comment.
or you can just omit those, they don't make sense for private functions
There was a problem hiding this comment.
Don't they? I'm big into static typing and these are definitely helpful when I'm reading code I didn't write, don't need to check any other callsite
There was a problem hiding this comment.
maybe for typespecs "don't make sense" is a stretch, since there won't be a compiler warning, but it's definitely uncommon to write typespecs for private functions. I think the reason is that you end up needing to check callsites anyway, since typespecs aren't enforced.
lib/posthog/feature_flags.ex
Outdated
|
|
||
| @doc false | ||
| @spec log_feature_flag_usage(supervisor_name(), String.t(), String.t(), {:ok, boolean() | String.t()} | {:error, Exception.t()}) :: :ok | ||
| defp log_feature_flag_usage(name, distinct_id, flag_name, result) |
There was a problem hiding this comment.
i don't think this clause is needed?
lib/posthog/feature_flags.ex
Outdated
| defp log_feature_flag_usage(name, distinct_id, flag_name, {:ok, variant}) do | ||
| PostHog.capture(name, "$feature_flag_called", %{ | ||
| distinct_id: distinct_id, | ||
| "$feature_flag": flag_name, | ||
| "$feature_flag_response": variant | ||
| }) | ||
|
|
||
| PostHog.set_context(name, %{"$feature/#{flag_name}" => variant}) | ||
| end | ||
|
|
||
| defp log_feature_flag_usage(_name, _distinct_id, _flag_name, {:error, _error}) do | ||
| # Do nothing for error cases | ||
| :ok | ||
| end |
There was a problem hiding this comment.
this looks awfully lot like a with statement it used to be!
There was a problem hiding this comment.
I know it does, but I consider extracting it "outside my view" to be a good practice here. When reading the main FF code I don't really need to know what "logging a feature flag usage" does, so it makes sense to extract it to a separate method over inlining it. If your comment is related to pattern-matching vs. with then I don't feel strong about it, however you prefer
There was a problem hiding this comment.
oh I don't mind moving it to a private function at all, just felt weird converting from a single with into 3 function clauses one of which never executes.
lib/posthog/feature_flags.ex
Outdated
| :ok | ||
| end | ||
|
|
||
| @doc false |
There was a problem hiding this comment.
sigh
fun fact, if you do write a @doc for a private function, compiler will issue a warning
| ** (PostHog.UnexpectedResponseError) Feature flag example-feature-flag-3 was not found in the response | ||
| """ | ||
| @spec check!(supervisor_name(), String.t(), distinct_id() | map() | nil) :: boolean() | String.t() | no_return() | ||
| def check!(name \\ PostHog, flag_name, distinct_id_or_body \\ nil) do |
There was a problem hiding this comment.
fun fact: this function doesn't work!
There was a problem hiding this comment.
duh, sorry.
I think we'd benefit a lot from doctests in this library but it might be hard because of the whole Supervisor dance, WDYT?
I'm sure it can be solved by configuring the supervisor/mocks at the module level when running doctests but I've never done it
There was a problem hiding this comment.
Hmm I can look into it, not sure how doctests work with setups 👀
README.md
Outdated
|
|
||
| ```shell | ||
| cp config/integration.example.exs config/integration.exs | ||
| cp config/test.exs config/dev.exs |
There was a problem hiding this comment.
hmm this will probably be quite confusing as dev.exs should probably be quite different from test.exs.
There was a problem hiding this comment.
Is that really true when we're building a library? I'd expect most of our local development to be based on the test suite over manual testing
There was a problem hiding this comment.
I don't see why not! Dropping into iex -S mix and playing with REPL is totally valid workflow IMO, just a very quick way to explore things.
Those suites can't run concurrently because they both spin up supervisor under the default PostHog name
|
Ok, this looks good to me! I'll be merging this to v2. Do you have anything else you'd need on this branch for your day job or is that all you'd need before you pointed your code to v2 vs. your fork @martosaur ? |
|
I'm actually rocking main repo v2.0.0 branch since we don't actually use feature flags yet. So I'll switch when this one is merged! So far we're all set. I have plans for introducing LLM Analytics helpers, but that doesn't block 2.0.0 release/release candidate! |
|
@martosaur Nice! Will merge this, add a couple of changelog entries over the weekend, and we can release it for real on Monday! I really appreciate your effort here, truly. |
Adding higher level functions for feature flags:
PostHog.flags/2thin wrapper over/flagsrequest, hence the name.PostHog.get_all_feature_flags/2- makes a request and returns portion of response that is under"flags"key.PostHog.check_feature_flag/3- our main start. It returns{:ok, true},{:ok, false}or{:ok, variant}if request was successful and a feature flag exists. otherwise returns{:error, Exception.t()}. Under the hood it also does all recommended actions: sends an event and sets feature flag in the context.Exception.t()means any error struct. If this is a request-level error, that could be aReq.TransportErrorfor example, or any other error client returns. If this is something we generate, for example ifdistinct_idis missing or feature flag wasn't found in the response, this would bePostHog.ErrororPostHog.UnexpectedResponseError.It's a little hard for me to tell how ergonomic this is, so would love to hear your thoughts.