Repository navigation
Conversation
Parse per-flag features from precomputed assignments, route them to flag metadata, exposure events and evaluation counts, and add an OpenFeature ExposureHook that shares the SDK's exposure decision. trackExposures(false) now stops only the send to Datadog; the exposure cache keeps recording.
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 9bf7d3e | Docs | View more details | Give us feedback! |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![datadog-official[bot]](https://avatars.githubusercontent.com/in/1358509?s=60&v=4){kind=small}
There was a problem hiding this comment.
Two correctness issues stand out: exposure callbacks can attribute the wrong subject when an OpenFeature context switch is in-flight (ExposureHook.kt), and nested flag evaluations during an after hook can evict the outer evaluation's pending decision, silently dropping its exposure record (ExposureDecisions.kt). A third issue in SplitFeaturesJson.kt allows out-of-range numeric feature values (e.g. 1e400) to overflow to Double.POSITIVE_INFINITY, corrupting telemetry JSON and changing the persisted value type.
🤖 Bits Code Review · Commit 9bf7d3e
| null | ||
| } | ||
| val targetingKey = ctx.ctx?.getTargetingKey() | ||
| val context = ctx.ctx?.let { ImmutableContext(it.getTargetingKey(), it.asMap()) } |
There was a problem hiding this comment.
With experimentation opt-out disabled, switching the OpenFeature context from subject A to B can retain A's assignments while B's refresh is pending or fails. The deferred Datadog exposure records A, but the callback uses B's OpenFeature context, pairing B with A's variant and deduplication decision. Carry the captured assignment context through the shared decision so warehouse and Datadog exposures identify the same subject.
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
| fun register(metadata: EvaluationMetadata, hookFeatures: Map<String, Any>, recordExposure: (() -> Boolean)?) { | ||
| if (hookFeatures.isEmpty() && recordExposure == null) return | ||
| val queue = decisions.get() ?: return | ||
| if (queue.size == MAX_DECISIONS_PER_THREAD) queue.removeFirstOrNull() |
There was a problem hiding this comment.
If an after hook performs 16 other evaluations on the same provider and thread, their completed decisions remain queued and evict the outer evaluation's pending decision. Its final provider hook then cannot record the Datadog exposure; ExposureHook also loses its features and can still report shouldSend() from provisional metadata. Retain pending decisions until their final hooks finish and clean up completed decisions through the evaluation lifecycle.
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
| when { | ||
| it.isString -> it.asString | ||
| it.isBoolean -> it.asBoolean | ||
| it.isNumber -> it.asDouble |
There was a problem hiding this comment.
A numeric feature such as 1e400 is valid JSON but overflows to Double.POSITIVE_INFINITY here. A focused reproduction with the repository's Gson 2.10.1 produced bare Infinity on serialization and reloaded it as a string. EXPOSURE or EVALUATION features therefore corrupt telemetry JSON, and persistence changes their value type. Skip non-finite conversions so unsupported features cannot damage the event or cached assignment.
| it.isNumber -> it.asDouble | |
| it.isNumber -> it.asDouble.takeIf { number -> number.isFinite() } |
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bf7d3e29f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| constructor(callback: ExposureCallback) : this( | ||
| callback, | ||
| { (Datadog.getInstance() as? FeatureSdkCore)?.internalLogger ?: InternalLogger.UNBOUND } |
There was a problem hiding this comment.
When the provider was created with a named SDK instance, this lookup always selects the default instance instead of the provider's sdkCore. A callback failure is therefore logged to another instance, or, if no default instance exists, triggers an unrelated “SDK not initialized” warning and then uses the unbound logger. Carry the provider's existing internalLogger through DatadogProviderMetadata and use it when handling callback failures.
AGENTS.md reference: AGENTS.md:L149-L150
Useful? React with 👍 / 👎.
| fun complete(details: FlagEvaluationDetails<*>): Boolean? = | ||
| if (details.errorCode != null || details.reason == Reason.ERROR.name) null else find(details)?.complete() |
There was a problem hiding this comment.
When an assignment has a blank variationKey or allocationKey, which the assignment schema accepts, resolveDeferringExposure still registers a recording closure whenever doLog is true. This method then invokes that closure because it checks only the final error state, so the SDK writes an exposure, while ExposureHook.createEvaluation rejects the same result because its variant or allocation metadata is null and reports isExposure() and shouldSend() as false. Gate recording on the same variant/allocation criteria used by the public verdict so the two stores cannot disagree.
Useful? React with 👍 / 👎.
| val isExposure = details.errorCode == null && | ||
| details.reason != Reason.ERROR.name && | ||
| details.variant != null && | ||
| allocationKey != null && | ||
| details.metadata.getBoolean(ExposureMetadata.DO_LOG) == true |
There was a problem hiding this comment.
When assignment extraLogging contains the newly reserved metadata keys, this metadata-only check can fabricate an exposure verdict. For example, a valid assignment with doLog=false but extraLogging values __dd_do_log=true and __dd_exposure_cache_hit=false makes ExposureHook report shouldSend() as true even though no recording closure was registered and the SDK sends nothing; the assignment schema allows arbitrary extraLogging properties. Base the verdict on the matched internal Decision, or always remove/overwrite these reserved keys before exposing metadata.
Useful? React with 👍 / 👎.
| writeExposureEvent(event) | ||
| val isRecorded = exposuresSentCache[cacheKey] == cacheValue | ||
| if (!isRecorded) { | ||
| writeExposureEvent(buildExposureEvent(flagKey, context, data)) |
There was a problem hiding this comment.
When trackExposures(false) is configured, FlagsFeature supplies a NoOpRecordWriter, but every first exposure still executes buildExposureEvent on the evaluating thread. That copies the complete context and allocates the generated event tree and routed feature map only for the writer to discard them; before this change, disabled exposure tracking skipped the processor entirely. Apps evaluating many distinct subject/flag pairs now pay avoidable hot-path work despite opting out, so cache admission should be separated from event construction when sending is disabled.
Useful? React with 👍 / 👎.
| internalLogger.log( | ||
| InternalLogger.Level.WARN, | ||
| listOf(InternalLogger.Target.MAINTAINER, InternalLogger.Target.TELEMETRY), | ||
| { "Ignoring unknown feature destination $destination" } | ||
| ) |
There was a problem hiding this comment.
When one response contains many distinct unknown destination values, this path emits a separate warning to both MAINTAINER and TELEMETRY for each value, up to 64 warnings per PrecomputeMapper. A malformed or forward-versioned configuration can therefore create a burst of 64 telemetry records during one mapping operation and repeat that burst for every client; per-string deduplication does not prevent the initial spam. Aggregate the destinations into one bounded warning or otherwise rate-limit this telemetry.
AGENTS.md reference: AGENTS.md:L149-L150
Useful? React with 👍 / 👎.
What does this PR do?
Precomputed assignments can carry
featuresfor the selected split. Each feature has akey, avalue(string, number or boolean) anddestinations(HOOK,EXPOSURE,EVALUATION). This PR uses them and lets applications record exposures in their own data store.featuresleniently. A malformed list or entry is skipped and never fails the flag. On a repeated key, the first valid entry wins. Unknown destinations are kept but ignored, with one warning for each distinct value (64 at most).HOOKfeatures in flag metadata as__dd_feature.<key>. This prefix is the only public metadata contract.EXPOSUREandEVALUATIONfeatures use internal prefixes.EXPOSUREfeatures to exposure events as a flatfeaturesobject. They are not part of the exposure cache entry.EVALUATIONfeatures to the evaluation-count bucket key and to each row, next to the consent dimension from #4025.ExposureHook. Its callback receives every completed evaluation withisExposure(),isCacheHit(),shouldSend(), theHOOKfeatures and a copy of the evaluation context. A failing callback does not affect the evaluation.ExposureHookshare it.trackExposures(false), the SDK sends no exposures but the exposure cache still records, soExposureHookgets correct decisions.Motivation
Customers who keep exposures in their own warehouse need each exposure once, with the same deduplication that Datadog uses, and the holdout fields of the selected split (
holdout.key,holdout.assignment_group,holdout.should_include_in_holdout_analysis). Some of them also need to stop sending exposures to Datadog.Additional Notes
This builds on the evaluation telemetry privacy work in #4025 by @vjfridge. Evaluation counts keep its consent dimension, protected targeting keys and error codes; split features are added next to them. OpenFeature structured results keep its metadata; this PR only adds the exposure decision to them.
UnparsedFlagand the bridge methods in_FlagsInternalProxyare unchanged, apart from one new internal method for the provider.Behaviour changes:
finallyAfterhook, only when the whole evaluation succeeded. Before, it sent the exposure at resolve time, also when a later hook failed.DatadogFlagsProvider.get...Evaluation()method, outside an OpenFeature client, records no exposure.MultiProvider(OpenFeature Kotlin SDK 0.7 and later) does not run provider hooks, so no exposures are recorded or sent under it, andExposureHookis not called. This matches the Go and browser SDKs.FlagsClient.resolve()and theresolve...Value()methods still record and send exposures immediately.trackExposures(false)now keeps the exposure cache running. It still sends nothing.Verified in the OpenFeature Kotlin SDK 0.6.2 used here (and 0.9.0): provider hooks run first in
finallyAfter; a failingbeforeorafterhook sets an error code beforefinallyAfter; an exception infinallyAfterreaches the application, soExposureHookcatches every callback exception.Open questions:
FlagsClientusers do not get an exposure callback. Is one needed?EXPOSUREandEVALUATIONfeatures do not reach Datadog through the bridge yet. A follow-up can add a features interface in the style ofUnparsedFlagWithConsent.ExposureHook,ExposureCallbackandExposureEvaluationnames.Overlap: #3924 adds
getLongEvaluation, which must use the same exposure path.Tests: unit tests in both Flags modules, and end-to-end tests in
reliability/single-fit/flagswith the real SDK core andOpenFeatureAPI. Each new test was checked by breaking the code under test.Review checklist (to be filled by reviewers)