chore: introduce classifications to dynamic configs #7616
Conversation
Signed-off-by: fimanishi <[email protected]>
fimanishi
requested review from
3vilhamster,
Shaddoll,
davidporter-id-au,
demirkayaender,
dkrotx,
jakobht,
neil-xie,
sankari165,
shijiesheng and
taylanisikdemir
as code owners
| ) | ||
|
|
||
| // Category indicates the purpose/nature of a config | ||
| type Category int |
There was a problem hiding this comment.
What do you think about making this a struct with some info available that may be passed in (such as a string for which release it's intended, or when it's being introduced or when it should be removed?)
c-warren
left a comment
c-warren
left a comment
There was a problem hiding this comment.
Overall I think this is a good idea. My only concern is that we're writing and maintaining these concepts within Cadence when the feature flag/dynamic config tool is the best place to be able to define these concepts. Keeping cadence 'dumb' here makes it easier to maintain, while the feature flag platform has a better ability to label/track stale gates and prompt the cadence owner to remove their stale gates.
| // LifecyclePermanent indicates a config expected to exist indefinitely | ||
| LifecyclePermanent | ||
| // LifecycleTemporary indicates a config that should be removed eventually | ||
| LifecycleTemporary |
There was a problem hiding this comment.
Do you think we'd be able to enforce an 'end date' for temporary flags? That would allow us to log whenever a flag is used (maybe debug normally, but an info or a warn after a flag has gone 'stale'). That would emulate something like this statsig feature: https://statsig.com/blog/removing-old-stale-feature-gates
There are some cons to this - it'll be hard to figure out up front how long you want the feature to exist (especially for some of our core feature flags), and we might have a bit of extra overhead with querying for stale flags often (maybe as part of on call).
But it would give us a good way to at least enforce that we should review stale flags.
| // Category indicates the purpose/nature of a config | ||
| type Category int | ||
|
|
||
| const ( |
There was a problem hiding this comment.
These are solid categories. Statsig has similar ideas with:
- Release instead of FeatureFlag
- Experiment
I think just adding experiment would be helpful here.
| CategoryFeatureFlag | ||
| // CategoryMigration indicates data migration or gradual rollout flags | ||
| CategoryMigration | ||
| // CategoryDebug indicates debugging/troubleshooting tools | ||
| CategoryDebug | ||
| // CategoryDeprecated indicates configs marked for removal (implies Temporary) |
There was a problem hiding this comment.
It would be nice to be able to enforce that these are temporary flags and not permanent (at least for migration/debug/experiment - feature flag I guess could conceivable be permanent, but at that point is it just an operatoinal flag?).
3 participants
What changed?
Added LifeCycle and Category classifications to dynamic configs.
#7607
Why?
Dynamic configs are used extensively in Cadence's code base. They can be important tools to set up a healthy service, deal with issues, rollout new features, etc.
We want users to be able to understand and use them better. That's why we are adding classifications.
We also want to rollout new features and remove temporary configs whenever possible. The LifeCycle type was created to help avoid stale configs and ensure that features are eventually released when testing is complete.
How did you test it?
N/A
Potential risks
Release notes
Documentation Changes