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

fix(flags): Add appropriate timeouts, fix some other misc things #44

Merged
merged 5 commits into from
Mar 15, 2024

Conversation

neilkakkar
Copy link
Contributor

@neilkakkar neilkakkar commented Mar 13, 2024

This PR does a few things:

  1. Add a feature_flag_request_timeout_seconds configuration option, defaulting to 3s to control flag timeouts
  2. Gets rid of a lot of error spew, instead allows passing in an on_error proc so users can decide what to do with errors
  3. Unifies calling sites such that everything doesn't call the /decide endpoint directly, but via get_all_flags_and_payloads which in turn does all necessary error handling. Before this, getting payloads would always throw :/
  4. Noticed as a result of above^ that we were sometimes inefficiently going to decide. The test updates where I add a second feature flag key for beta-feature2 reflect that, since previously this would go to decide, but now it wouldn't, which is infact the ideal thing to do. (and thus to test the old flow we need an extra flag that is present but can't be locally evaluated).

Noticed while doing this that the above problem exist in posthog-python as well, will update that too

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.

Some ruby thoughts, but 🚢

@neilkakkar neilkakkar merged commit e048b8b into master Mar 15, 2024
5 checks passed
@neilkakkar neilkakkar deleted the timeout-fix branch March 15, 2024 13:37
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