Page MenuHomePhabricator

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

Authored by tomek on Feb 23 2023, 8:31 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Feb 21, 2:04 PM
Unknown Object (File)
Wed, Feb 21, 2:04 PM
Unknown Object (File)
Wed, Feb 21, 2:04 PM
Unknown Object (File)
Wed, Feb 21, 9:05 AM
Unknown Object (File)
Wed, Feb 21, 9:05 AM
Unknown Object (File)
Wed, Feb 21, 9:05 AM
Unknown Object (File)
Mon, Feb 12, 6:52 AM
Unknown Object (File)
Mon, Feb 12, 3:58 AM
Subscribers

Details

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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek requested review of this revision.Feb 23 2023, 8:46 AM
services/feature-flags/src/service.rs
30 ↗(On Diff #23012)

Maybe a better name would be feature_name_if_enabled. The check_if_sth suggests it returns a boolean

38 ↗(On Diff #23012)

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.

services/feature-flags/src/service.rs
30 ↗(On Diff #23012)

Right!

38 ↗(On Diff #23012)

Ok, will give it a try.

Rebase and simplify code version check

This revision is now accepted and ready to land.Feb 27 2023, 5:05 AM