Skip to content

fix: harden dependent flag failure handling - #453

Draft
typotter wants to merge 2 commits into
mainfrom
dependent-flags-precomputed-web
Draft

typotter wants to merge 2 commits into
mainfrom
dependent-flags-precomputed-web

Conversation

@typotter

@typotter typotter commented Nov 4, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Applies the evaluator findings synthesized from Aaron’s dependent-flags reviews across SDK implementations.

  • normalize dependency-graph failures to GENERAL on the requested/root flag
  • preserve each failed prerequisite evaluation’s specific error code and message
  • include the failing prerequisite key in the root error message
  • tolerate malformed evaluatorParams blocks and invalid or wrongly typed maxDependencyDepth values by logging an error and using the hard-coded depth of 10
  • accept unknown future fields in JSON flagEvaluation objects while continuing to require a non-empty key
  • preserve valid 0 through 255 depth overrides

Scope

This PR changes evaluator parsing and failure semantics only. It does not implement precomputed payload generation, the new precomputed error wire shape, or obfuscated-key encryption. Existing precomputation regression tests are updated only because they exercise the shared evaluator and must distinguish root errors from prerequisite diagnostics.

Validation

  • focused core dependent-flags tests: 60 passed
  • focused Node dependency-event tests: 17 passed
  • full core suite: 1,327 passed, 6 skipped
  • full Node server suite: 737 passed
  • full browser suite: 648 passed
  • full build and type checks
  • Biome lint and Prettier format checks
  • package build tests: 126 passed
  • git diff --check

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved prerequisite-handling defects drop feature metadata and assigned values, mishandle obfuscated lookups, and cause inconsistent exposure recording, targeting context, and cache behavior.

6 open issues
What changed in this PR

Adds dependent-flag support to precomputed assignments, extending the prerequisite provenance work in #437 into browser evaluation, exposure reporting, and RUM enrichment.

Changes:

  • Adds dependency records, error results, validation, and capability negotiation for precomputed assignments.
  • Reports ordered prerequisite evaluations while limiting exposures and RUM enrichment to selected dependencies.
  • Adds browser corpus and dependency tests, and preserves Node prerequisite telemetry when later hooks fail.
File Description
packages/​node-server/​test/​dependent-flags.spec.ts Tests telemetry preservation after hook failures.
packages/​node-server/​src/​provider.ts Separates dependency telemetry retention from exposure rollback.
packages/​node-server/​rules-based.d.ts Updates precomputed assignment declarations.
packages/​node-server/​index.d.ts Updates telemetry sidecar declarations.
packages/​core/​test/​evaluation/​precomputed-dependencies.spec.ts Tests dependency reconstruction and validation.
packages/​core/​src/​evaluation/​precomputed-evaluation.ts Attaches dependency records to evaluation results.
packages/​core/​src/​evaluation/​precomputed-dependencies.ts Reconstructs and stores prerequisite details.
packages/​core/​src/​evaluation/​index.ts Exports dependency helpers.
packages/​core/​src/​configuration/​wire.test.ts Adapts assertions to the assignment union.
packages/​core/​src/​configuration/​wire-validation.ts Validates error assignments and dependency records.
packages/​core/​src/​configuration/​flag-key-obfuscation.ts Advertises dependent-flag capability.
packages/​core/​src/​configuration/​configuration.ts Defines dependency and error assignment types.
packages/​core/​README.md Documents precomputed dependency semantics.
packages/​browser/​test/​transport/​fetchConfiguration.spec.ts Updates capability-header expectations.
packages/​browser/​test/​openfeature/​rumIntegration.spec.ts Tests selected-dependency RUM enrichment.
packages/​browser/​test/​openfeature/​provider.spec.ts Updates provider capability expectations.
packages/​browser/​test/​openfeature/​precomputed-corpus.spec.ts Adds a browser runner for Web-enabled shared fixtures.
packages/​browser/​test/​openfeature/​obfuscation.spec.ts Updates capability expectations for obfuscated responses.
packages/​browser/​test/​openfeature/​flagEvaluations.spec.ts Tests prerequisite telemetry ordering.
packages/​browser/​test/​openfeature/​exposures.spec.ts Tests selected-dependency exposure emission.
packages/​browser/​test/​openfeature/​exposure-hook.spec.ts Tests dependency callbacks and shared exposure decisions.
packages/​browser/​src/​openfeature/​rumIntegration.ts Enriches RUM with selected prerequisites.
packages/​browser/​src/​openfeature/​flagEvaluations.ts Records prerequisite evaluations before the root.
packages/​browser/​src/​openfeature/​exposures.ts Prepares and emits dependency exposures.
packages/​browser/​src/​openfeature/​exposure-hook.ts Delivers selected dependencies to customer callbacks.
packages/​browser/​src/​openfeature/​core-provider-metadata.ts Preserves dependency associations when copying metadata.

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@github-actions

github-actions Bot commented Nov 4, 2026 •

Copy link
Copy Markdown

OpenFeature Browser Provider Bundle Sizes

Measured from the Vite production output after installing packed @datadog/flagging-core and @datadog/openfeature-browser tarballs.

Both scenarios initialize an OpenFeature provider, evaluate a boolean flag, change context, and evaluate again. Telemetry is disabled for DatadogProvider; no tracking hooks are registered for DatadogCoreProvider. DatadogProvider fetches precomputed assignments for each context; DatadogCoreProvider receives rules from fetchRulesConfiguration once and evaluates locally.

Sizes include OpenFeature and the same small scenario harness. Configuration responses are supplied by Playwright and are not bundled. These are complete scenario JS sizes, not configuration payload sizes or isolated provider/Protobuf costs; the difference between rows is not a decoder-only delta.

Scenario HTML JS Assets Raw JS Gzip JS Protobuf Markers
DatadogProvider (precomputed fetching) provider.html 15 107.5 KiB 38.8 KiB no
DatadogCoreProvider + fetchRulesConfiguration core-provider.html 15 207.6 KiB 66.3 KiB yes

Dependency checks passed: no Protobuf markers in default/precomputed scenarios; markers present in rules-based scenarios.

OpenFeature Browser Tracking Hook Bundle Sizes

Synthetic entrypoints import and call tracking hook factories from the packed @datadog/openfeature-browser/rules-based ESM package. Deltas are measured against the no-hook baseline from the same Vite production build.

Scenario HTML JS Assets Raw JS Raw Δ Gzip JS Gzip Δ
tracking hook baseline tracking-baseline.html 5 2.7 KiB 0 B 1.5 KiB 0 B
exposure logging hook tracking-exposure.html 10 71.3 KiB +68.6 KiB 26.4 KiB +24.9 KiB
evaluation logging hook tracking-evaluation.html 11 67.1 KiB +64.4 KiB 25.0 KiB +23.5 KiB
RUM tracking hook tracking-rum.html 8 6.8 KiB +4.1 KiB 3.5 KiB +2.0 KiB
all tracking hooks tracking-all.html 13 76.1 KiB +73.4 KiB 28.6 KiB +27.1 KiB

OpenFeature Node Server Bundle Sizes

Independent Webpack production builds of the installed core and Node SDK tarballs, targeting Node 18 with ESM (module export condition). Node built-ins remain external. OpenFeature, dd-trace, and configuration fixtures are not bundled.

Scenario Raw JS Gzip JS Protobuf Modules
Node provider only 56.8 KiB 17.0 KiB no
SSR helpers only (fetch client rules + serialize) 84.8 KiB 24.9 KiB yes
Node provider + SSR helpers 139.9 KiB 40.6 KiB yes
Precompute only (parsed rules) 36.0 KiB 11.8 KiB no
Node provider + precompute 60.0 KiB 18.0 KiB no

Adding SSR helpers to the provider adds 83.1 KiB raw / 23.6 KiB gzip in this build. This includes fetching, parsing, and serialization, not just the decoder. SSR helpers fetch rules for the browser, not for configuring that provider (which uses its existing JSON API).

Adding precomputation to the provider adds 3.2 KiB raw / 1.0 KiB gzip in this build. Both precompute scenarios import precomputeConfiguration from /rules-based and receive already-parsed rules from the test harness; they do not fetch, parse, or serialize configuration.

These are bundler sizes, not npm install sizes, unbundled Node require/import costs, payload sizes, or latency. Gzip compares compressed artifacts. Deltas compare scenarios in this build, not changes against the base branch. The browser scenarios include OpenFeature and a different harness, so their sizes are not directly comparable.

Dependency checks passed: no Protobuf runtime or generated schema modules in the provider-only or either precompute bundle; present in both SSR bundles (checked using Webpack module statistics).

This report shows current PR artifact sizes only; it does not compare against the base branch.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The browser path can suppress exposure callbacks when context is unreadable and lose prerequisite telemetry when a later after hook throws.

3 open issues

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Human assessment is needed because an unreadable context getter can silently skip all prerequisite exposure callbacks even when the root tracking context resolves successfully.

No open issues 🎉

3 resolved since last review

Review effort: Balanced

@typotter

typotter commented Nov 4, 2026

Copy link
Copy Markdown
Collaborator Author

Fresh Copilot review reports no open issues. On the remaining “closer look” note: the custom exposure hook resolves the tracking context once, catches an unreadable caller context, and still iterates every prerequisite plus the root callback with an undefined cloned context. The lifecycle regression suite covers that path, the independent local review found no material issue, and the full browser suite passes (625 tests).

@aarsilv aarsilv added the crucible: reviewing A Crucible review has started; this PR has reviewer attention. label Nov 4, 2026
Base automatically changed from dependent-flags-protobuf-provenance to main November 4, 2026 20:06
Comment thread packages/node-server/src/provider.ts Resolved
@aarsilv

aarsilv commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

🤖 Comment from Claude working with Aaron Silverman:

[P2] Dependency depth limit (core evaluator on main, from #414; posted here because #414 has merged and this PR is the open follow-up). Please choose a common lower maximum dependency depth across all evaluators, for example 32 instead of 255.

All evaluators in this stack accept 0–255 today. In libdatadog (DataDog/libdatadog#2748), a 255-edge chain overflows 128 KiB and 256 KiB thread stacks in a release build, and Rust aborts the process on a stack overflow. I suggested lowering the maximum there to 32. All evaluators should keep the same range, so that one configuration behaves the same in every SDK.

Suggested change in packages/core/src/evaluation/evaluation.ts: numericDepth <= 32, update the "0 through 255" message and its assertions in dependent-flags.spec.ts, and make the same change in Go, Java, .NET, the fixtures and the design document.

Comment thread packages/node-server/src/provider.ts Resolved
Comment thread packages/core/src/evaluation/rules.ts Resolved
Normalize root dependency failures while preserving prerequisite details, tolerate future dependency fields, and default malformed evaluator options safely.

Environment: Datadog workspace

Co-Authored-By: Codex GPT-5 <noreply@localhost>
@typotter
typotter force-pushed the dependent-flags-precomputed-web branch from 36398b6 to e078194 Compare November 5, 2026 16:16
@typotter typotter changed the title feat: support dependent flags in precomputed assignments fix: harden dependent flag failure handling Nov 5, 2026
Reject bigint depth overrides on JSON configurations while preserving protobuf bigint support, and strengthen depth-boundary and dependency telemetry coverage.

Environment: Datadog workspace

Co-Authored-By: Codex GPT-5 <noreply@localhost>
source.evaluatorParams = fromJson(FlagsConfigurationSchema, {
evaluatorParams: { maxDependencyDepth: failure === 'depth limit' ? 0 : 256 },
}).evaluatorParams
['missing parent', 'GENERAL', 'FLAG_NOT_FOUND', undefined],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Comment from Claude working with Aaron Silverman:

[P2] Follow-up to our root error code comment: evaluate() now returns GENERAL for the root, but the precompute path still reports the prerequisite's code for the same root. This table now encodes that split.

  • precompute-configuration.ts:95-98 pushes error.errorCode, which is the prerequisite's code. For a missing parent, onEvaluationError gets FLAG_NOT_FOUND, and for an invalid parent PARSE_ERROR, while evaluate() returns GENERAL for the same root. A precomputed producer that writes the root's error from this callback gets the prerequisite's code.
  • Our earlier comment also asked for GENERAL on failed ancestors. The intermediate prerequisite still copies the leaf's code: packages/core/test/evaluation/dependent-flags.spec.ts:670 expects FLAG_NOT_FOUND for prerequisite when only missing is absent.

Suggested change (tested: with it, precompute-configuration.test.ts and dependent-flags.spec.ts pass, 122 tests, and tsc --noEmit passes):

// precompute-configuration.ts
errorCode: error instanceof DependencyGraphError ? error.rootErrorCode : ('GENERAL' as ErrorCode),
// this table
['missing parent', 'GENERAL', 'GENERAL', undefined],
['invalid parent', 'GENERAL', 'GENERAL', 'PARSE_ERROR'],

}

function logInvalidEvaluatorParams(logger: Logger, rootFlagKey: string): void {
logger.error('invalid evaluatorParams; using default maxDependencyDepth', {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Comment from Claude working with Aaron Silverman:

[P3] With invalid evaluatorParams, this logs at error level on every root evaluation that reaches a dependency.

  • getMaxDependencyDepth runs once when a root evaluation first reaches a dependency (lines 380 and 473), and each invalid case calls this function.
  • On main, an invalid depth made the dependent evaluation fail with PARSE_ERROR. Now evaluation continues with the default depth of 10, and the log is the only signal of the invalid parameters. It repeats on each evaluation that reaches a dependency. A probe with maxDependencyDepth: 256 and 1,000 evaluations of one root gave 1,000 logger.error calls.

Suggested change: log once per configuration object. Tested: the same probe logs once, precompute-configuration.test.ts and dependent-flags.spec.ts pass (122 tests), and tsc --noEmit passes.

const reportedInvalidEvaluatorParams = new WeakSet<object>()

function logInvalidEvaluatorParams(logger: Logger, config: object, rootFlagKey: string): void {
  if (reportedInvalidEvaluatorParams.has(config)) return
  reportedInvalidEvaluatorParams.add(config)
  logger.error('invalid evaluatorParams; using default maxDependencyDepth', {
    flagKey: rootFlagKey,
    defaultMaxDependencyDepth: DEFAULT_MAX_DEPENDENCY_DEPTH,
  })
}

Then pass config at the four call sites in getMaxDependencyDepth.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

crucible: reviewing A Crucible review has started; this PR has reviewer attention.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants