feat(flags): Basic flags service #31
Conversation
| // .allow_origin(AllowOrigin::mirror_request()); | ||
|
|
||
| let router = Router::new() | ||
| .route("/flags", post(v0_endpoint::flags).get(v0_endpoint::flags)) |
There was a problem hiding this comment.
endpoint name tbd
| shutdown: Arc<Notify>, | ||
| } | ||
|
|
||
| impl ServerHandle { |
There was a problem hiding this comment.
I tried not re-building this and just using axum-test, but ran into annoyances there around the IP extraction not working well, what capture does sans axum-test-helper indeed seems the most flexible + useful here 👍
There was a problem hiding this comment.
axum-test-helper is currently outdated and we switched to a fork but should move away from it anyway.
It's short enough to not bother trying to make it generic, let's just copy it as you did.
There was a problem hiding this comment.
I meant this one: https://docs.rs/axum-test/latest/axum_test/ but indeed control over all the boilerplate here, given its not a lot is 👍
| shutdown: Arc<Notify>, | ||
| } | ||
|
|
||
| impl ServerHandle { |
There was a problem hiding this comment.
axum-test-helper is currently outdated and we switched to a fork but should move away from it anyway.
It's short enough to not bother trying to make it generic, let's just copy it as you did.
| // TODO: Find if there's a better way around this. | ||
| // mockall got really annoying with async and results so I'm just gonna do my own | ||
| #[derive(Clone)] | ||
| pub struct MockRedisClient { |
There was a problem hiding this comment.
By default, all cargo tests in a module run concurrently, so you indeed want isolation. Mocking in rust is hard, so I recommend using the real data stores from the compose stack:
- for Redis, my recommendation is to have the
RedisClienthave a configurable key prefix (passed as aOption<String>) that it adds on all commands. This way, we can generate random prefixes and clean them onDrop. Happy to pair on that. - for PG, you should be able to get enough isolation by generating new teams?
There was a problem hiding this comment.
I think I don't even need to go that far for redis, since all redis keys I care about are scoped by team tokens anyway, so generating a new team ought to be sufficient for both redis & pg.
Will dive into this more once I actually connect - but I do like your idea of not using these mocks - just copied this for now to get a feel for how capture does it, but indeed not too happy with these mocks right now.
| // .allow_origin(AllowOrigin::mirror_request()); | ||
|
|
||
| let router = Router::new() | ||
| .route("/flags", post(v0_endpoint::flags).get(v0_endpoint::flags)) |
There was a problem hiding this comment.
you'll need to wire readiness, liveness and metrics too. Can be done later.
There was a problem hiding this comment.
Indeed. Added to list here: PostHog/posthog#22131
feature-flags/tests/common.rs
Outdated
| @@ -28,7 +28,7 @@ pub struct ServerHandle { | |||
|
|
|||
| impl ServerHandle { | |||
| pub async fn for_config(config: Config) -> ServerHandle { | |||
| let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); | |||
| let listener = TcpListener::bind("127.0.0.1:3001").await.unwrap(); | |||
There was a problem hiding this comment.
Port zero is intentional, it tells the OS to assign a random port that's available, avoiding collision with other processes and other tests in parallel. That's why we then call let addr = listener.local_addr().unwrap(); to get the port to query.
There was a problem hiding this comment.
whoops, great catch, thanks! I was trying something here, forgot to revert 🤦
| tracing::Span::current().record("version", meta.version.clone()); | ||
| tracing::Span::current().record("method", method.as_str()); | ||
| tracing::Span::current().record("path", path.as_str().trim_end_matches('/')); | ||
| tracing::Span::current().record("ip", ip.to_string()); |
There was a problem hiding this comment.
Might be useful to annotate the span with the distinct_id too.
There was a problem hiding this comment.
absolutely, will clean this up, and add this once I add functionality to extract distinct id
| .get("content-type") | ||
| .map_or("", |v| v.to_str().unwrap_or("")) | ||
| { | ||
| "application/x-www-form-urlencoded" => { |
There was a problem hiding this comment.
I wonder if this might be better off just using an if statement outside this match block with condition for application/json content-type.
There was a problem hiding this comment.
yeah good shout, since I'm not going to handle anything else in v1. Will do this in a follow up where I add all parsing req logic
|
|
||
| let token = request.extract_and_verify_token()?; | ||
|
|
||
| tracing::Span::current().record("token", &token); |
There was a problem hiding this comment.
Would this be the API token sent by the client? Maybe we can log the sha256 hash of the token instead of the actual token since that's PII.
There was a problem hiding this comment.
Not really, these are public tokens which determine which team to send data to - we want them to correlate data with the right team.
| #[derive(Debug, PartialEq, Eq, Deserialize, Serialize)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct FlagsResponse { | ||
| pub error_while_computing_flags: bool, |
There was a problem hiding this comment.
We should consider making this a string and renaming the field to just error
There was a problem hiding this comment.
.. to give client more info about the error?
This would usually only be a db error, so internal, not very useful to expose to clients.
For now I want the same shape as the existing endpoint, so will keep this as is
Please be as brutal and nitty as you can be with the reviews, you'll help me learn rust faster :D
This is mostly a boiler plate copy from the capture endpoint which sets up a basic server + route responses. Most of this will change, will get refactored once I have the actual flags service running, but until then this seemed like a reasonable place to begin.