docs: add adr for targeting metadata - #2013
Conversation
dosubot
Bot
added
the
size:M
✅ Deploy Preview for polite-licorice-3db33c ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
f6c88cc to
986a12c
Compare
📝 WalkthroughWalkthroughAdds an architecture decision for targeting results that support existing variant strings or objects with a ChangesTargeting variant metadata
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
df15c70 to
79f3a25
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/architecture-decisions/targeting-variant-metadata.md`:
- Around line 23-24: Revise the metadata plumbing statement to distinguish the
currently supported paths: confirm metadata for OFREP single/bulk and single
gRPC responses, note that this proto layer’s ResolveAll path does not yet expose
it, and remove the claim that RecordEvaluation telemetry already carries
metadata attributes. Limit the ADR’s end-to-end contract to these verified paths
and identify metrics/telemetry support as a separate change.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e45a8980-8e18-4872-8ea0-f2e078b09b03
📒 Files selected for processing (1)
docs/architecture-decisions/targeting-variant-metadata.md
| The plumbing to carry metadata already exists end to end: `AnyValue.Metadata` (`core/pkg/evaluator/ievaluator.go`) is `map[string]interface{}`, threaded through every resolver path and out through gRPC and OFREP as evaluation metadata that OpenFeature SDKs surface as `flagMetadata`. | ||
| The only thing missing is a way for a *rule branch* to contribute to it. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 \
'value\.Metadata|Metadata:|RecordEvaluation|AnyFlag|flagMetadata|OFREP' \
--glob '*.go' --glob '*.proto' --glob '*.json' . || trueRepository: open-feature/flagd
Length of output: 50375
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== candidate files =="
git ls-files | rg 'flag_evaluator.*\.go$|ofrep|ievaluator|metrics|resol' | sed -n '1,200p'
echo
echo "== relevant source slices =="
sed -n '60,145p' flagd/pkg/service/flag-evaluation/flag_evaluator.go
sed -n '130,145p' flagd/pkg/service/flag-evaluation/flag_evaluator_v1.go
sed -n '360,405p' flagd/pkg/service/flag-evaluation/flag_evaluator.go
sed -n '245,270p' flagd/pkg/service/flag-evaluation/flag_evaluator_v2.go
echo
echo "== OFREP response metadata occurrences =="
rg -n -C 5 'metadata|Metadata|Resolve.*Response|flagMetadata|flag_metadata|recordEvaluation|Evaluation\(' flagd/pkg/service/ofrep **/ofrep*.go --glob '!*vendor*' --glob '!*testdata*' --glob '!*test' || trueRepository: open-feature/flagd
Length of output: 7340
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== OFREP handler/source slices =="
sed -n '1,260p' flagd/pkg/service/flag-evaluation/ofrep/handler.go
sed -n '1,220p' flagd/pkg/service/flag-evaluation/ofrep/ofrep_service.go
echo
echo "== schema V1 Proto AnyFlag messages =="
rg -n -C 4 'message AnyFlag|field Metadata|Metadata' proto schema flagd --glob '*.proto' --glob '*.pb.go' --glob '*.go' || true
echo
echo "== response result interface implementations =="
rg -n -C 4 'SetResult|RecordEvaluation|func .*SetResult|RecordEvaluation\(' flagd core flagd-proxy --glob '*.go' --glob '!*vendor*' || trueRepository: open-feature/flagd
Length of output: 50375
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== OFREP response model slices =="
sed -n '1,260p' core/pkg/service/ofrep/models.go
echo
echo "== OFREP test cases around metadata =="
sed -n '1,320p' flagd/pkg/service/flag-evaluation/ofrep/handler_test.go
sed -n '1,160p' core/pkg/service/ofrep/models_test.go
echo
echo "== OFREP service serializer occurrences =="
rg -n -C 4 'SuccessResponseFrom|BulkEvaluationResponseFrom|AnyValue|Metadata|flagMetadata|flag_metadata' core/pkg/service/ofrep --glob '*.go' || trueRepository: open-feature/flagd
Length of output: 33279
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== RecordEvaluation definitions =="
sed -n '1,220p' core/pkg/telemetry/metrics.go
rg -n -C 6 'func .*RecordEvaluation|RecordEvaluation.*Metadata|metadata|Metadata' core pkg flagd --glob '*.go' --glob '!*test.go' | sed -n '1,240p'
echo
echo "== AnyFlag schema declarations in vendored/generated/proto deps =="
rg -n -C 4 'message AnyFlag|AnyFlag|Metadata.*Struct|struct FieldMetadata' .vendor $GOPATH vendor flagd --glob '*.go' --glob '*.proto' --glob '*.pb.go' 2>/dev/null | sed -n '1,240p' || trueRepository: open-feature/flagd
Length of output: 26109
Clarify the metadata contract before calling the path end to end.
OFREP single and bulk responses carry AnyValue.Metadata, and single gRPC responses carry Metadata; ResolveAll now carries it through the v1 schema but not ResolveAll in this proto layer. RecordEvaluation(...) also does not expose metadata attributes yet, so the metrics/telemetry part of the ADR needs a separate change.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/architecture-decisions/targeting-variant-metadata.md` around lines 23 -
24, Revise the metadata plumbing statement to distinguish the currently
supported paths: confirm metadata for OFREP single/bulk and single gRPC
responses, note that this proto layer’s ResolveAll path does not yet expose it,
and remove the claim that RecordEvaluation telemetry already carries metadata
attributes. Limit the ADR’s end-to-end contract to these verified paths and
identify metrics/telemetry support as a separate change.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
I generally like the idea, but I am missing the migration path. How do we plan to move from old version to new version, without breaking migration patterns? I feel like we need to point out how we plan to migrate without breaking installations for others. In-process providers might be used in deprecated services and we need to give people time to migrate from one to the other. Is it maybe time to have a version contract in the request for the flag configuration? |
Signed-off-by: Parth Suthar <parth.suthar@dynatrace.com>
79f3a25 to
490029e
Compare
dosubot
Bot
added
size:L
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/architecture-decisions/targeting-variant-metadata.md`:
- Around line 29-45: Update the ADR to document the migration contract for the
new tagged-object return shape, including upgrade order, behavior when an older
provider rejects it or returns PARSE_ERROR, and whether configuration requests
include a schema or feature version. Clarify that existing string configurations
remain compatible but the new format is not forward-compatible, and remove any
claim of full backward compatibility until mixed-version rollout is defined.
- Around line 47-51: Extend integration tests around evaluateVariant to cover
string results, tagged objects with and without metadata, nested if branches,
and invalid result shapes. Ensure single-key {"variant":...} objects are
supported despite jsonlogic/v3.9.1 treating them as operators by adjusting
evaluateVariant or the dependency, while preserving PARSE_ERROR for unsupported
shapes and validating metadata merging.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f2ca8f5-e081-4794-b186-cbf48a00637b
📒 Files selected for processing (1)
docs/architecture-decisions/targeting-variant-metadata.md
| Extend `definitions.primitive` in `schemas/json/targeting.json` with a tagged-object return shape: | ||
|
|
||
| ```json | ||
| { | ||
| "type": "object", | ||
| "required": ["variant"], | ||
| "additionalProperties": false, | ||
| "properties": { | ||
| "variant": { "type": "string" }, | ||
| "metadata": { | ||
| "$ref": "https://flagd.dev/schema/v0/flags.json#/definitions/metadata" | ||
| } | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| Reusing the existing `metadata` definition keeps values restricted to `string | number | boolean`, matching flag and flag-set metadata. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Document the migration and configuration-version contract before accepting this ADR.
When an older provider receives the new object return shape, it may reject the configuration or return PARSE_ERROR. Existing string configurations remain compatible, but the new configuration format is not forward-compatible with older providers.
Document the upgrade order, unsupported-provider behavior, and whether configuration requests carry a schema or feature version. Do not describe this as fully backwards compatible until mixed-version installations have a defined rollout path.
Also applies to: 47-51, 138-140
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/architecture-decisions/targeting-variant-metadata.md` around lines 29 -
45, Update the ADR to document the migration contract for the new tagged-object
return shape, including upgrade order, behavior when an older provider rejects
it or returns PARSE_ERROR, and whether configuration requests include a schema
or feature version. Clarify that existing string configurations remain
compatible but the new format is not forward-compatible, and remove any claim of
full backward compatibility until mixed-version rollout is defined.
| In `evaluateVariant` (`core/pkg/evaluator/json.go`), replace the current string-strip of the JsonLogic result with a typed decode: | ||
|
|
||
| - string → variant key, as today | ||
| - object with `variant` field → use that as the variant key; shallow-merge `metadata` into the metadata already being returned | ||
| - anything else → `PARSE_ERROR`, same as an unrecognized return today |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== JsonLogic module declaration =="
rg -n 'github.com/diegoholiveira/jsonlogic' . \
--glob 'go.mod' \
--glob 'go.sum' || true
echo "== evaluator implementation and tests =="
rg -n -C 8 'evaluateVariant|jsonlogic|PARSE_ERROR' core \
--glob '*.go'Repository: open-feature/flagd
Length of output: 24028
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== current evaluator conversion path =="
sed -n '326,420p' core/pkg/evaluator/json.go
echo "== evaluator test files and relevant test declarations =="
git ls-files 'core/pkg/evaluator/*_test.go'
rg -n -C 5 'evaluateVariant|targeting|ParseErrorCode|PARSE_ERROR|metadata' core/pkg/evaluator --glob '*_test.go' | head -n 260
echo "== module declaration =="
sed -n '1,24p' core/go.mod
echo "== available local jsonlogic source, if any =="
find "${GOMODCACHE:-/nonexistent}" -path '*diegoholiveira*jsonlogic*' -type f 2>/dev/null | head -n 40 || trueRepository: open-feature/flagd
Length of output: 22003
🌐 Web query:
github.com/diegoholiveira/jsonlogic v3.9.1 Apply operator-free object literal behavior
💡 Result:
The version v3.9.1 of the diegoholiveira/jsonlogic Go library, released on April 30, 2026, includes various internal fixes [1], but the specific phrase "operator-free object literal behavior" is not explicitly mentioned in the official release notes [1][2]. However, based on the implementation details within the library, JSON objects are processed according to the following logic: 1. Identification of Operators: The library identifies operators by evaluating the map keys [3][4]. If a map contains exactly one key that matches a registered operator, it executes that operation [3][5]. 2. Operator-Free Objects: In the context of this library, if a JSON object (a map) has more than one key, or if the key does not match any registered operators, it is treated as a primitive (or a raw object literal) and returned as-is rather than being interpreted as a JsonLogic command [3][4]. This behavior allows the library to distinguish between JSON structures intended as logic rules and standard JSON objects that should be passed through as data literals [3]. This mechanism serves as a way to "skip" evaluation for objects that do not conform to the expected { "operator": [...] } rule format [3][4]. { "value": "The v3.9.1 release of the Go implementation of JsonLogic (diegoholiveira/jsonlogic) focuses on small fixes and internal improvements, including those related to operator evaluation and short-circuit logic [1][2]. While the specific phrase 'operator-free object literal behavior' is not explicitly defined in the official documentation, the library handles such structures by checking the number of keys and valid operators in a map. If a map contains multiple keys or an unrecognized operator, it is treated as a literal and returned without further recursive processing [3][4]. This design choice is fundamental to how the library prevents standard data objects from being incorrectly parsed as logic commands [3].", "citations": [3][4][1][2], "confidence": "high"}
Citations:
- 1: https://github.com/diegoholiveira/jsonlogic/releases/tag/v3.9.1
- 2: diegoholiveira/jsonlogic@v3.9.0...v3.9.1
- 3: https://github.com/diegoholiveira/jsonlogic/blob/master/jsonlogic.go
- 4: https://github.com/diegoholiveira/jsonlogic/blob/main/validator.go
- 5: https://github.com/diegoholiveira/jsonlogic/blob/main/operation.go
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
base='https://raw.githubusercontent.com/diegoholiveira/jsonlogic/v3.9.1'
echo "== upstream evaluator implementation =="
curl -fsSL "$base/jsonlogic.go" | nl -ba | sed -n '1,260p'
echo "== upstream validator and operation handling =="
for file in validator.go operation.go; do
echo "--- $file ---"
curl -fsSL "$base/$file" | nl -ba | sed -n '1,240p'
done
echo "== upstream tests mentioning object literals, if, or Apply =="
for file in jsonlogic_test.go evaluator_test.go validator_test.go; do
body="$(curl -fsSL "$base/$file" 2>/dev/null || true)"
if [ -n "$body" ]; then
echo "--- $file ---"
printf '%s\n' "$body" | rg -n -C 4 'object|literal|Apply|if|map|variant' | head -n 220 || true
fi
doneRepository: open-feature/flagd
Length of output: 309
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
base='https://raw.githubusercontent.com/diegoholiveira/jsonlogic/v3.9.1'
echo "== upstream evaluator implementation =="
curl -fsSL "$base/jsonlogic.go" | sed -n '1,280p'
echo "== upstream validator and operation handling =="
for file in validator.go operation.go; do
echo "--- $file ---"
curl -fsSL "$base/$file" | sed -n '1,260p'
done
echo "== upstream repository file list =="
curl -fsSL 'https://api.github.com/repos/diegoholiveira/jsonlogic/git/trees/v3.9.1?recursive=1' |
grep -o '"path": "[^"]*"' |
grep -E 'test|jsonlogic.go|validator.go|operation.go' |
head -n 100Repository: open-feature/flagd
Length of output: 11512
Add compatibility tests for tagged objects.
jsonlogic/v3.9.1 treats only maps with more than one key as literals. A {"variant":"blue"} result is treated as operator variant, so jsonlogic.Apply returns an error and evaluateVariant returns PARSE_ERROR. Add integration coverage through evaluateVariant for string returns, tagged objects with and without metadata, nested if branches, and invalid shapes. Adjust the evaluator or dependency if single-key tagged objects are supported.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/architecture-decisions/targeting-variant-metadata.md` around lines 47 -
51, Extend integration tests around evaluateVariant to cover string results,
tagged objects with and without metadata, nested if branches, and invalid result
shapes. Ensure single-key {"variant":...} objects are supported despite
jsonlogic/v3.9.1 treating them as operators by adjusting evaluateVariant or the
dependency, while preserving PARSE_ERROR for unsupported shapes and validating
metadata merging.
2 participants
This PR
Related Issues
Notes
Follow-up Tasks
How to test