docs: ADR for updated disabled flag behaviour#1919
docs: ADR for updated disabled flag behaviour#1919suthar26 wants to merge 2 commits intoopen-feature:mainfrom
Conversation
✅ Deploy Preview for polite-licorice-3db33c ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
dosubot
Bot
added
the
size:L
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an Architecture Decision Record (ADR) to change flagd's handling of disabled flags. Instead of returning an error, disabled flags will now return a successful evaluation with a reason of DISABLED and the defaultVariant value, aligning with the OpenFeature specification. Feedback on the ADR highlights several technical implementation details that need to be addressed: the logic in evaluateVariant should be simplified, and the resolve[T] function must be updated to handle DISABLED reasons for empty variants to avoid type mismatch errors. Additionally, the proposal needs to account for gRPC v1 compatibility in bulk evaluations and include specific implementation steps in the timeline for the gRPC and OFREP service layers.
suthar26
force-pushed
the
docs/adr-disabled-flag-evaluation
branch
5 times, most recently
from
73a3398 to
c5259ee
Compare
suthar26
force-pushed
the
docs/adr-disabled-flag-evaluation
branch
from
c5259ee to
31200d1
Compare
toddbaert
changed the title
toddbaert
requested review from
alxckn,
askpt,
beeme1mr,
cupofcat,
erka and
juanparadox
| --- | ||
| # Treat Disabled Flag Evaluation as Successful with Reason DISABLED | ||
|
|
||
| This ADR proposes changing flagd's handling of disabled flags from returning an error (`reason=ERROR`, `errorCode=FLAG_DISABLED`) to returning a successful evaluation with `reason=DISABLED` and the flag's `defaultVariant` value. A flag that does not exist in a flag set should remain a `FLAG_NOT_FOUND` error. |
There was a problem hiding this comment.
I'm not sure about this.
I would expect the behavior to be return the code-default in all cases when the flags is disabled. Otherwise I'm not really sure what the point is. If we use the defaultVariant, we are still delegating the evaluation do the flag management system, which contradicts the DISABLED idea to me.
I think we just just immediately return the code default.
Am I not thinking about this correctly? Maybe you had a different concept in mind.
There was a problem hiding this comment.
i agree, it should use the code_default/fallback - the barriers in flagd are blurry, because if there is no default_variant on the flag, we also use the code_default. but in the case of a disabled flag, it should be the code_default
There was a problem hiding this comment.
no you are totally right, thats what we should be doing
toddbaert
left a comment
toddbaert
left a comment
There was a problem hiding this comment.
Review focusing on in-process provider coverage, OFREP spec alignment details, and format.
There was a problem hiding this comment.
I think this is a bit of a blocker for me; it seems a bit incoherent. Maybe I'm missing something though or not thinking about it correctly. I think if this was changed this would be an easy approval for me. Please help me understand if I'm off-base 😅
suthar26
force-pushed
the
docs/adr-disabled-flag-evaluation
branch
3 times, most recently
from
9dcffbd to
a4f89fb
Compare
Signed-off-by: Parth Suthar <parth.suthar@dynatrace.com>
Signed-off-by: Parth Suthar <parth.suthar@dynatrace.com>
suthar26
force-pushed
the
docs/adr-disabled-flag-evaluation
branch
from
a4f89fb to
7b4902b
Compare
|
| The response has the following properties: | ||
|
|
||
| - **value**: Omitted (signals code-default deferral). The SDK/provider uses the application's code-defined default value. | ||
| - **variant**: Omitted (empty string). |
There was a problem hiding this comment.
Why an empty string? I'd expect this to be completely omitted in supported languages.
| ### Timeline | ||
|
|
||
| 1. Update `evaluateVariant` in `core/pkg/evaluator/json.go` to return `reason=DISABLED` with an empty variant (code-default deferral) instead of an error | ||
| 2. Update `resolve[T]` in `core/pkg/evaluator/json.go` to handle `DisabledReason` with empty variants (avoids `TYPE_MISMATCH`) | ||
| 3. Remove the disabled-flag skip in `ResolveAllValues` and update the `default` branch of the type switch to include disabled flags for gRPC v1 requests | ||
| 4. Update `SuccessResponseFrom` in `core/pkg/service/ofrep/models.go` to preserve `reason=DISABLED` (with field omission) for disabled flags deferring to code defaults | ||
| 5. Remove `FlagDisabledErrorCode` handling from `errFormat`, `errFormatV2`, and `EvaluationErrorResponseFrom` | ||
| 6. Update gRPC v1 service layer (`flag_evaluator_v1.go`) to handle nil values in `ResolveAll` responses for disabled flags deferring to code defaults | ||
| 7. Update each language SDK's in-process provider evaluator to return `reason=DISABLED` with code-default deferral instead of raising an error when encountering a disabled flag | ||
| 8. Update flagd-testbed with test cases for disabled flag evaluation across all surfaces | ||
| 9. Update OpenFeature providers to recognize `DISABLED` as a non-error, non-cacheable reason | ||
| 10. Update provider documentation and migration guides |
| ### Open questions | ||
|
|
||
| - Should bulk evaluation include an option to exclude disabled flags for clients that prefer the current behavior (smaller payloads)? | ||
| - How should existing dashboards and alerts that key on `FLAG_DISABLED` errors be migrated? Should we provide a deprecation period where both behaviors are available? |
There was a problem hiding this comment.
I don't think this is something we can do. We just need to make it clear that's in the release notes that we're making the behavior change.
|
|
||
| - Should bulk evaluation include an option to exclude disabled flags for clients that prefer the current behavior (smaller payloads)? | ||
| - How should existing dashboards and alerts that key on `FLAG_DISABLED` errors be migrated? Should we provide a deprecation period where both behaviors are available? | ||
| - Does this change require a new flagd major version, or can it be introduced in a minor version with appropriate documentation given the spec alignment argument? |
There was a problem hiding this comment.
It should be marked as a behavior-breaking change, which will update the minor version since we're sub-1.0.
| - Should bulk evaluation include an option to exclude disabled flags for clients that prefer the current behavior (smaller payloads)? | ||
| - How should existing dashboards and alerts that key on `FLAG_DISABLED` errors be migrated? Should we provide a deprecation period where both behaviors are available? | ||
| - Does this change require a new flagd major version, or can it be introduced in a minor version with appropriate documentation given the spec alignment argument? | ||
| - Should the `FlagDisabledErrorCode` constant be retained (but unused) for a deprecation period, or removed immediately? |
4 participants
This PR
Related Issues
Fixes #1918
Notes
Follow-up Tasks
How to test