Repository navigation
Send feature flag exposures after evaluation, and let applications record them with ExposureHook - #12774
Conversation
The provider now deduplicates exposures itself and sends them from a finally-stage hook instead of at resolution. Resolution only checks the new provider cache and stamps __dd_exposure_cache_hit in the flag metadata. The hook records the exposure and sends it when the allocation logs exposures, the subject is not already exposed, and the evaluation has no error, so an evaluation that an application hook fails no longer sends an exposure or marks the subject as exposed. The agent's writer keeps its own cache for now, because an older provider still sends every evaluation.
Test each condition under which the exposure hook sends nothing, and list the exposure cache's key and value classes as POJOs excluded from coverage verification, as the writer's cache classes are.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
SpotBugs reports IA_AMBIGUOUS_INVOCATION_OF_INHERITED_OR_OUTER_METHOD because the anonymous LinkedHashMap's size() call could resolve to the cache's own size(). A static nested map class has no outer instance, so the call is unambiguous.
This comment has been minimized.
This comment has been minimized.
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 268677e167
ℹ️ 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".
There was a problem hiding this comment.
Logging allocations can fail on older agents because the new cache lookup reads an unsupported serial-ID field before checking compatibility. The provider cache can also suppress exposures after concurrent dispatch reordering or queue rejection.
🤖 Bits Code Review · Commit 268677e
The exposure cache check read split.serialId before exposureSerialId could apply SPLIT_SERIAL_ID_SUPPORTED. With an older dd-java-agent whose Split has no serialId field, every evaluation of an allocation that logs exposures then threw NoSuchFieldError. The read now sits behind the same guard as the other serial id reads.
Exposures are now sent from a provider hook. OpenFeature SDK 1.20.1 to 1.22.1 does not run the hooks of providers wrapped in a MultiProvider, so a Datadog provider inside one sent no exposures. SDK 1.23.0 runs them. The README states 1.23.0 as the minimum for MultiProvider, and a test covers the MultiProvider path; it fails on 1.20.1.
The ImmutableMetadata getters build a debug message for every absent key, whether or not debug logging is on. __dd_do_log is absent from most evaluations by default, so the hook built that string on most of them. It now reads the keys from asUnmodifiableMap().
The flag now names the capability it detects, like SPLIT_SERIAL_ID_SUPPORTED next to it, instead of the case to avoid. Its value is inverted, and every reader is inverted with it.
They have the same size but refresh entries at different times, so once full they can evict different subjects. The comment said they evict the same ones.
leoromanovsky
left a comment
There was a problem hiding this comment.
Catching up; solid iteration and noticing the OF bump needed.
aarsilv
left a comment
There was a problem hiding this comment.
Thanks for iterating! There a few lower priority comments love for you to look at, but nothing blocking.
On dd-java-agent 1.65 and 1.66 the lookup at resolution uses no serial id, so the hook must record none either. Otherwise every repeat of a split with a serial id is sent again.
google-java-format had split it, leaving a line that held only one word.
) * Parse features on UFC splits A UFC split can carry a list of key-value features for exposure hooks. Split gains a nullable features field, and the parser reads it leniently: a malformed list or entry is dropped instead of rejecting the flag, so features never change which variation a subject gets. * Cover the features adapter factory and exclude the Feature POJO from coverage Test the factory's type and annotation checks and the read-only adapter, as the other UFC adapters do, and list the Feature model class with the other UFC POJOs that are excluded from coverage verification. * Let applications record feature flag exposures with ExposureHook Add a public ExposureHook that hands each completed evaluation to a callback with the provider's exposure decision. shouldSend() is true for exactly the exposures the provider sends to Datadog. getFeatures() returns the selected split's features, which the provider now copies into the flag metadata as __dd_feature.<key>. DD_FEATURE_FLAGS_EXPOSURES_DATADOG_LOGGING_ENABLED=false stops sending exposures to Datadog. The exposure cache keeps running, so ExposureHook callbacks get the same decisions either way. * Parse the destinations of UFC split features Each feature now lists where SDKs deliver it, such as HOOK, EXPOSURE or EVALUATION. Destinations are open strings: the parser keeps every non-empty string, including ones this SDK does not know, and drops a feature that has no destinations. * Deliver only HOOK features to exposure hooks Copy a split feature into the __dd_feature.<key> metadata only when its destinations include HOOK. EXPOSURE and EVALUATION are known destinations that are not delivered to hooks. A destination the SDK does not know is ignored, with one warning for each new name. * Replace getCacheHit() with a primitive isCacheHit() SpotBugs reports NP_BOOLEAN_RETURN_NULL because getCacheHit() returned a Boolean that is null when the evaluation is not an exposure. A caller that unboxes it gets a NullPointerException. isCacheHit() returns false in that case; isExposure() still tells callers whether it is one. shouldSend() still needs an explicit false stamp, so sending is unchanged. * Cap the unknown feature destinations the evaluator remembers The warn-once set kept every unknown destination name for the life of the application, so a stream of configurations with new names grew it without bound. It now stops at 64 names; later unknown destinations are still ignored, only without a warning. * State when shouldSend() matches Datadog, and surface hook failures shouldSend() matches the exposures Datadog receives only when the provider's own hooks run. The Javadoc now says so, names the MultiProvider case before OpenFeature SDK 1.23.0, and says that ExposureHook is optional because the flag metadata carries the same data. ExposureHook now skips the callback when OpenFeature passes no details, and logs the first callback failure at warn level instead of debug. ExposureLoggingHook logs its first LinkageError at warn level instead of swallowing it. Both read flag metadata through its map, because the ImmutableMetadata getters build a debug message for every absent key. * Test that the Datadog exposure logging setting turns sending off The earlier tests passed false straight to the gate or the hook, so a wrong key would still pass. This forked test sets the real system property and checks that the provider sends nothing to Datadog while ExposureHook keeps its decisions. * Lead the exposure README with the flag metadata any hook can read The README now says first that the provider puts its exposure decision and the split's features in the flag metadata, and that ExposureHook is a convenience on top of it. It says how to read the metadata, narrows the shouldSend() claim to when the provider's hooks run, corrects the __dd_do_log and __dd_split_serial_id rows, and adds a table of the dd-java-agent version each capability needs. * Test evaluation with an agent whose Split has no features No released dd-java-agent has Split.features yet. The provider must skip reading it there, or every evaluation throws NoSuchFieldError. * Correct when shouldSend() matches what Datadog receives It matches only while Datadog exposure logging is on, and a changed serial id counts only with dd-java-agent 1.67.0 or later. The README also gives 1.68.0 for split features, says that the logging setting does not stop flag evaluation events, and that the provider does not read dd-trace-py's DD_FFE_INTAKE_ENABLED. * Send EXPOSURE and EVALUATION feature flag features to Datadog (#12797) * Send EXPOSURE and EVALUATION features to Datadog A UFC split feature can list the EXPOSURE and EVALUATION destinations. The evaluator now writes each feature once for each destination it lists, under that destination's metadata prefix. The exposure hook puts EXPOSURE features on the exposure event, and the flag-evaluation hook puts EVALUATION features on the flag-evaluation event, whatever the consent, because they come from the flag configuration. Flag-evaluation buckets now include the features in both tiers, so rows with different features never merge, and each row serializes them. Both events gain a features field with new constructors; with an older agent the provider sends them without features. * Keep the first feature when a split repeats a feature key The FFE_FLAGS schema documents that keys are unique within a split and that SDKs keep the first feature with a repeated key. Flag metadata is a map, so the provider used to keep the last one. It now ignores a later feature with a key it has already seen, for every destination. * Check flag-evaluation feature support inside FlagEvalLoggingHook DDEvaluator resolved FlagEvalEvent.class in its static initializer. Agents released before FlagEvalEvent existed then failed DDEvaluator's initialization with NoClassDefFoundError, which broke the provider. The check now lives in FlagEvalLoggingHook, the only class that uses it, and Provider already guards that hook's loading against LinkageError. * Read split features without DDEvaluator, and scan only when needed - Moves featuresWithPrefix into SplitFeatureMetadata, which depends only on the OpenFeature API. ExposureHook.getFeatures() called it on DDEvaluator, which needs the Datadog Java agent, so it threw NoClassDefFoundError where the agent is absent, such as unit tests. - DDEvaluator marks the metadata with __dd_evaluation_features only when the split routes a feature to EVALUATION. FlagEvalLoggingHook scans the metadata only then, instead of on every evaluation. - Adds an end-to-end test of EVALUATION features for an allocation that does not log exposures. Every earlier provider-backed test used doLog=true, so adding features only for logged allocations passed. - Reflows a broken comment. * Share one copy of each feature set across a flush window Each bucket kept the features map that the provider built for its own evaluation, and each key kept its own canonical string. Full-tier keys include the targeting key, so up to PER_FLAG_CAP subjects of one split held that many equal maps and strings until the flush. The aggregator now keeps one key and map for each distinct feature set, and clears them with the rest of the window. * Test that ExposureHook works without the Datadog Java agent The test classpath always has the agent classes, so no other test sees ExposureHook reach for them. This test loads ExposureHook in a class loader that hides DDEvaluator and the agent classes, as in an application without the agent. * Read flag evaluation metadata from its map, and rename the marker - FlagEvalLoggingHook reads allocationKey, the evaluation timestamp, the consent key and the evaluation-features marker from one map. The ImmutableMetadata getters build a debug message for every absent key, and error and default results carry only the consent key. - Renames the marker to __dd_has_evaluation_features, so that it does not differ from the __dd_evaluation_feature. prefix by one character. - Rewraps a comment that google-java-format had split.
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
Build pipeline has failing jobs for 009d6b8: What to do next?
DetailsSince those jobs are not marked as being allowed to fail, the pipeline will most likely fail. |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
What Does This Do
This PR combines #12775 and #12797, which were reviewed and approved separately and squashed into this branch.
Exposures after a successful evaluation
ExposureLoggingHook, at the OpenFeature finally stage. A failed evaluation sends nothing.ExposureDeduplicationCache. Resolution checks it and stamps__dd_exposure_cache_hit; the hook records the exposure and sends it.MultiProvider.ExposureHook (#12775)
ExposureHook, which hands each completed evaluation to a callback with the provider's exposure decision (isExposure(),isCacheHit(),shouldSend(),getFeatures()).HOOKfeatures into the flag metadata as__dd_feature.<key>.DD_FEATURE_FLAGS_EXPOSURES_DATADOG_LOGGING_ENABLED(defaulttrue). Whenfalse, the provider sends no exposures to Datadog, and the exposure cache keeps running.EXPOSURE and EVALUATION features (#12797)
EXPOSUREfeatures on exposure events and itsEVALUATIONfeatures on flag evaluation events. Flag evaluation buckets include the features.dd-java-agent, the provider checks for each capability and sends events without features.Motivation
Some customers use Datadog feature flags but keep exposures in their own data store. They need each exposure, with the holdout key and assignment group, in their own code, and they need to stop sending the same exposures to Datadog. Datadog also routes other features, such as a bandit policy id, to exposures and flag evaluations.
Additional Notes
dd-openfeature) and the agent (dd-java-agent) ship separately.Providerevaluation methods directly runs no hooks.EXPOSUREorEVALUATIONuntil ddoghq/dd-source#124673 is deployed in every datacenter.Jira ticket: EX-3800 EX-3815 EX-3801 EX-3828