feat(broker): add feature flag template

[pihme]

Description

Adds template to add feature flags. The template consists of two parts:

• FeatureFlags to hold the flags and default values. Visible in most of the code base
• FeatureFlagsCfg to make feature flags configurable. Instances of this class can be used to obtain a FeatureFlags object

Review Hints

• I did try to implement reflection based tests to check that e.g. the cfg object and the feature flags object have the same getters, and that values are passed in correctly. However, this spiraled out of control into a big chunk of over-engineered tests. So I dropped these completely
• Secondly there are often config tests like ExperimentalCfgTest not sure if these add value. Let me know if you miss them as part of the template
• Overall, this is a minimal implementation. Please let me know if you want to change any names or such things.

Related issues

closes #9254

Definition of Done

Code changes:

• The changes are backwards compatibility with previous versions
• If it fixes a bug then PRs are created to backport the fix to the last two minor versions. You can trigger a backport by assigning labels (e.g. backport stable/1.3) to the PR, in case that fails you need to create backports manually.

Testing:

• There are unit/integration tests that verify all acceptance criterias of the issue
• New tests are written to ensure backwards compatibility with further versions
• The behavior is tested manually
• The change has been verified by a QA run
• The impact of the changes is verified by a benchmark

Documentation:

• The documentation is updated (e.g. BPMN reference, configuration, examples, get-started guides, etc.)
• New content is added to the release announcement
• If the PR changes how BPMN processes are validated (e.g. support new BPMN element) then the Camunda modeling team should be informed to adjust the BPMN linting.

Please refer to our review guidelines.

[npepinpe]

👍

I think for the tests, we can leave it to implement on a feature-per-feature basis. Whoever implements a feature with a flag should test that the default value, then both cases where it's enabled/disabled. I would hope that covers all of it and we don't need a test to ensure that both the config and the flags produced are the same.

I think name-wise everything is fine. So I would configure things as:

zeebe:
  broker:
    experimental:
      features:
        enableFoo: true

Or

export ZEEBE_BROKER_EXPERIMENTAL_FEATURES_ENABLEFOO=true

Which seems fine to me. It's a departure from having the flag nested with the feature's own configuration. One hand it's easy to discover experimental features - on the other hand, you then have to find the configuration for that feature somewhere else. I think with comments this is a non-issue though.

[npepinpe]

🙃 I assume the class is there so the first person to add a flag will remember to add a test. I would personally do away with it and trust we'll add it when we need it, or we'll add per-feature tests, but I don't have super strong arguments. ❓ I am curious though in case I misunderstood the intent here.

[pihme]

Yeah, it's a bit flimsy. What I had in mind is testing the default values of the feature flags.
So you start with assertThat(sut.myFeature()).isFalse(); and later you set it to assertThat(sut.myFeature()).isTrue();

And at some point you would see something like:

assertThat(sut.myFeature()).isTrue();
assertThat(sut.mySecondFeature()).isFalse();
assertThat(sut.myUrgentFeature()).isTrue();

Which gives you a bit of a safety net against messing up the order in the constructor. At the same time, this test is so indirect it is questionable whether it is useful. I would keep it for now, but if Philip also votes to remove it, it's gone

[saig0]

@pihme looks good for me 👍

[pihme]

Which seems fine to me. It's a departure from having the flag nested with the feature's own configuration. One hand it's easy to discover experimental features - on the other hand, you then have to find the configuration for that feature somewhere else

I was thinking more about feature flags the don't need configuration. For those with configuration, I am unsure whether I want to put the config alongside the flag, or put it somewhere else.

[pihme]

bors merge

[npepinpe]

I was thinking more about feature flags the don't need configuration. For those with configuration, I am unsure whether I want to put the config alongside the flag, or put it somewhere else.

I'd be fine trying whichever way, I think this is something we'll figure out in practice. But I imagine it'll be a question the first time someone wants to add a flag. 🤷‍♂️

[pihme]

@npepinpe Could you please have a look at this Wiki page? https://github.com/camunda/zeebe/wiki/Feature-Flags

I think it is still a little light on rollout steps and maybe we can also add links to runbooks or something like that for how to change a feature flag for a cluster on int stage.

[npepinpe]

I can add it to our cloud get together agenda - the experience of rolling out flags to prod has been somewhat painful, so I'd like to get the SREs involved in the discussion since they also feel this pain

[zeebe-bors-camunda]

Build succeeded:

• continuous-integration/jenkins/branch
• Smoke tests on windows-latest
• Smoke tests on ubuntu-latest
• Smoke tests on macos-latest

[pihme]

We decided to backport these changes in order to make future backports of bugfiyes easier

[saig0]

/backport

[saig0]

/backport

[github-actions]

Successfully created backport PR #9274 for stable/8.0.

[github-actions]

Backport failed for stable/1.3, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin stable/1.3
git worktree add -d .worktree/backport-9257-to-stable/1.3 origin/stable/1.3
cd .worktree/backport-9257-to-stable/1.3
git checkout -b backport-9257-to-stable/1.3
ancref=$(git merge-base 5c35438f4c2918c7964b831ec6dbdcb38e7cbf84 a1a5c656c44313920376560fb597f2d499ef4a0d)
git cherry-pick -x $ancref..a1a5c656c44313920376560fb597f2d499ef4a0d

[github-actions]

Backport failed for stable/8.0, because it was unable to create a new branch.

Please cherry-pick the changes locally.

git fetch origin stable/8.0
git worktree add -d .worktree/backport-9257-to-stable/8.0 origin/stable/8.0
cd .worktree/backport-9257-to-stable/8.0
git checkout -b backport-9257-to-stable/8.0
ancref=$(git merge-base 5c35438f4c2918c7964b831ec6dbdcb38e7cbf84 a1a5c656c44313920376560fb597f2d499ef4a0d)
git cherry-pick -x $ancref..a1a5c656c44313920376560fb597f2d499ef4a0d