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

feat(flags): more flags production-readiness #26998

Merged
merged 34 commits into from
Jan 26, 2025
Merged

Conversation

dmarticus
Copy link
Contributor

@dmarticus dmarticus commented Dec 18, 2024

Changes

  • add docker topology for exposing the flags service behind port 8000 (like the rest of the app)
  • error_while_computing_flags -> errors_while_computing_flags (to match current behavior)
  • broke flag_request into flag_request and flag_service, where the request module just handles the request, and the service handles fetching data from sources. This will make it so that we don't go to any databases, etc if we have missing/malformed data in the request
  • added metrics for my black hole service to confirm that request parsing is working as expected
  • return flag payloads (totally overlooked that when focusing on evaluation, whoops)
  • handle date operator comparison

Does this work well for both Cloud and self-hosted?

No impact yet, since nothing is hitting the /flags endpoint.

How did you test this code?

Have been locally testing it by creating flags locally, pointing the SDK at the flags endpoint, and confirming that they evaluate as expected.

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week. If you want to permanentely keep it open, use the waiting label.

@posthog-bot
Copy link
Contributor

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.

@posthog-bot posthog-bot closed this Jan 1, 2025
@dmarticus dmarticus reopened this Jan 1, 2025
@posthog-bot posthog-bot removed the stale label Jan 2, 2025
@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week. If you want to permanentely keep it open, use the waiting label.

@posthog-bot posthog-bot removed the stale label Jan 14, 2025
@dmarticus dmarticus marked this pull request as ready for review January 17, 2025 23:53
Comment on lines +32 to +35
@flags {
path /flags
path /flags*
}
Copy link
Contributor Author

@dmarticus dmarticus Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this puts the flags service behind the reverse proxy that's handling our other rust services.

Comment on lines +45 to +47
handle @flags {
reverse_proxy feature-flags:3001
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we run on port 3001, and then route it to port 8000

Comment on lines +56 to +60
pub async fn options() -> Result<Json<FlagsOptionsResponse>, FlagError> {
Ok(Json(FlagsOptionsResponse {
status: FlagsResponseCode::Ok,
}))
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +16 to +17
#[error("billing limit reached")]
BillingLimit,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will implement the quota limiting in another PR; I started on it and then it got kinda beefy. Leaving this in for now though since it's harmless to add the error type, the PR will land shortly anyway.

Gzip,
Base64,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we explicitly won't support b64 encoding for new flags traffic; gzip or bust babay

@@ -0,0 +1,118 @@
use axum::{
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this file is for testing request traffic without hitting any of the services; not helpful for testing evaluation, but helpful for testing that the various types of requests that we can send are handled correctly.

pub feature_flag_payloads: HashMap<String, Value>, // flag key -> payload
pub feature_flag_payloads: HashMap<String, Value>,
#[serde(skip_serializing_if = "Option::is_none")]
pub quota_limited: Option<Vec<String>>, // list of quota limited resources
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will add quota limiting in a future PR

@@ -0,0 +1,388 @@
use common_metrics::inc;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

none of this is new code, it was just pulled out of flag_request along with the tests. Should work fine.

Comment on lines -18 to -28
// TODO: the following fields are used for the `/decide` response,
// but they're not used for flags and they don't live in redis.
// At some point I'll need to differentiate between teams in Redis and teams
// with additional fields in Postgres, since the Postgres team is a superset of the fields
// we use for flags, anyway.
// pub surveys_opt_in: bool,
// pub heatmaps_opt_in: bool,
// pub capture_performance_opt_in: bool,
// pub autocapture_web_vitals_opt_in: bool,
// pub autocapture_opt_out: bool,
// pub autocapture_exceptions_opt_in: bool,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RemoteConfig will handle decide, so I'm explicitly not going to parse any of this stuff from our teams table for flags.

@dmarticus dmarticus requested a review from a team January 23, 2025 05:00
Copy link
Contributor

@havenbarnes havenbarnes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@haacked haacked left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is my first time reading Rust, yet I felt like I could follow along. I appreciate the clarity of the code. Code looks good to me, but I wouldn't trust that I would notice a bug given my lack of Rust experience. 😄

@dmarticus dmarticus merged commit c5762a3 into master Jan 26, 2025
93 checks passed
@dmarticus dmarticus deleted the feat/flags-to-prod branch January 26, 2025 22:51
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.

4 participants