[services][feature-flags] Determine enabled features set
ClosedPublic
Actions

Authored by tomek on Feb 23 2023, 8:31 AM.

Details

Reviewers

michal
bartek
• jon
varun

Commits

rCOMMc2bb76aec775: [services][feature-flags] Determine enabled features set

Summary

Determine a set of enabled features based on the provided params. The idea is described in https://linear.app/comm/issue/ENG-2614/specify-requirements-of-feature-flags-service but the most important part is that when determining a config we take it from the newest version not younger that the version we check.

Depends on D6857

Test Plan

Called the function and checked if it returns correct value depending on the params. It is though a good idea to have some unit tests for it.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

tomek created this revision.Feb 23 2023, 8:31 AM

Herald added subscribers: atul, ashoat. · View Herald TranscriptFeb 23 2023, 8:31 AM

tomek added a child revision: D6859: [services][feature-flags] Add configuration for http server.Feb 23 2023, 8:33 AM

Harbormaster completed remote builds in B16833: Diff 23012.Feb 23 2023, 8:46 AM

tomek requested review of this revision.Feb 23 2023, 8:46 AM

bartek added inline comments.Feb 24 2023, 2:20 AM

services/feature-flags/src/service.rs
30	Maybe a better name would be `feature_name_if_enabled`. The `check_if_sth` suggests it returns a boolean
38	IIRC, the `version` here is of type `&i32`, but primitives should be OK to access by value - my suggestion should also work.

agree with bartek, something name <object>_is_<condition>, should probably return a boolean, or be renamed to more accurately describe what is going on.

In the future, we may want to think about doing some memoization of feature flags; as they probably won't change significantly between calls (staff may change, but I don't have the context why staff are handled special here).

In the future, we may want to think about doing some memoization of feature flags; as they probably won't change significantly between calls (staff may change, but I don't have the context why staff are handled special here).

Response caching is a great idea! A few first thoughts:

Likely many devices would perform the same request and receive the same response over and over again
The cache could be an in-memory key-value pair, could be separate for each replica or possibly some Redis instance
The key consists of platform&staff&version, values are features
The cache could be simply invalidated when we update some features
The motivation for this is to decrease the number of DDB reads (and save some money) and possibly improve response times

Anyway we could create a Linear task to discuss this further

I've created a task where we can discuss the caching https://linear.app/comm/issue/ENG-3082/consider-using-cache-in-feature-flags-service. I think it is really not worth it in near future, as it greatly increases the complexity and doesn't introduce measurable value.