Repository navigation
FFLSDK-254: Add a first-flags callback to the Android Flags client - #3945
gh-worker-dd-mergequeue-cf854d[bot] merged 27 commits into
Conversation
|
✅ All CI checks and tests passed. Datadog automation helped this PR pass. 🎉 All green!🧪 All tests passed 🔄 Datadog retried 1 test - 1 passed on retry 🎯 Code Coverage (details) 🔗 Commit SHA: b02d400 | Docs | View more details | Give us feedback! |
a08f580 to
44e4e3e
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Cache misses and failures now cause repeated synchronous 100 ms read delays until flags are successfully installed.
Review effort: Balanced
Findings: 1
What changed in this PR
Adds a one-shot Android Flags callback for the first installed cache or network configuration, independent of client readiness.
Changes:
- Adds immutable events and
FlagsClient.onFirstFlags(). - Retains first-installation keys for late registrations.
- Adds tests, documentation, sample usage, and Detekt configuration.
| File | Description |
|---|---|
| sample/kotlin/src/test/kotlin/com/datadog/android/sample/flags/FirstFlagsSampleTest.kt | Tests sample logging and evaluation. |
| sample/kotlin/src/main/kotlin/com/datadog/android/sample/SampleApplication.kt | Registers the sample callback. |
| sample/kotlin/src/main/kotlin/com/datadog/android/sample/flags/OpenFeatureFragment.kt | Saves a Boolean flag selection. |
| sample/kotlin/src/main/kotlin/com/datadog/android/sample/flags/FirstFlagsSample.kt | Logs keys and evaluates the saved flag. |
| sample/kotlin/build.gradle.kts | Adds sample test dependencies. |
| features/dd-sdk-android-flags/src/test/kotlin/com/datadog/android/flags/model/FlagsClientEventTest.kt | Tests event immutability and shape. |
| features/dd-sdk-android-flags/src/test/kotlin/com/datadog/android/flags/internal/repository/FirstFlagsLatchTest.kt | Tests latch delivery and concurrency. |
| features/dd-sdk-android-flags/src/test/kotlin/com/datadog/android/flags/internal/repository/FirstFlagsInstallationTest.kt | Tests cache/network installation ordering. |
| features/dd-sdk-android-flags/src/test/kotlin/com/datadog/android/flags/FirstFlagsIntegrationTest.kt | Tests real-client callback behavior. |
| features/dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/model/FlagsClientEventType.kt | Defines the configuration event type. |
| features/dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/model/FlagsClientEvent.kt | Adds immutable event values. |
| features/dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/internal/repository/FlagsRepository.kt | Exposes the installation latch internally. |
| features/dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/internal/repository/FirstFlagsLatch.kt | Retains first keys and delivers listeners. |
| features/dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/internal/repository/DefaultFlagsRepository.kt | Signals first installation and changes read waiting. |
| features/dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/internal/NoOpFlagsClient.kt | Handles callback registration without installations. |
| features/dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/internal/DatadogFlagsClient.kt | Delivers retained events and isolates exceptions. |
| features/dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/FlagsClient.kt | Declares and documents the callback API. |
| features/dd-sdk-android-flags/README.md | Documents events and sample usage. |
| features/dd-sdk-android-flags/api/dd-sdk-android-flags.api | Records binary API additions. |
| features/dd-sdk-android-flags/api/apiSurface | Records public API additions. |
| detekt_custom_safe_calls_third_party.yml | Allows the atomic installation operation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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. |
aarsilv
left a comment
aarsilv
left a comment
There was a problem hiding this comment.
Thanks for iterating! Deleting FirstFlagsIntegrationTest leaves the real client's onFirstFlags() untested, but this looks by design as you want to move it elsewhere 👌
0xnm
left a comment
0xnm
left a comment
There was a problem hiding this comment.
The blocker for me is the Collections.unmodifiableList() usage, especially in the public API of FlagsClientEvent.
| /** Snapshot of supplied keys, or null when keys were not supplied. */ | ||
| // ArrayList and unmodifiableList reject null inputs; let supplies non-null keys and the copy is non-null. | ||
| @Suppress("UnsafeThirdPartyFunctionCall") | ||
| val flagsChanged: List<String>? = flagsChanged?.let { Collections.unmodifiableList(ArrayList(it)) } |
There was a problem hiding this comment.
This AI comment is wrong. What we are exposing in the public API now is java.util.List which has methods in the definition allowing to mutate the returned collection, while it is actually unmodifiable. Calling such mutable methods will throw UnsupportedOperationException. I don't think this is what users expect to discover in production while doing wrong thing.
If simply suggestion is used, then kotlin.collections.List is returned, which doesn't have such methods, all methods there are read-only.
The argument of AI about cast is very weak, and loses to the better type definition (and more Kotlin-friendly) of kotlin.collections.List.
55d94f6
0xnm
left a comment
0xnm
left a comment
There was a problem hiding this comment.
I don't have knowledge about domain-specific logic (and I didn't go through threading model in details as well), but code-wise change looks okay.
|
Thanks @0xnm!! Just need one last stamp for the conflict-resolving merge. |
![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: b02d400f02
ℹ️ 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".
| flagsChanged: List<String>? = null | ||
| ) { | ||
| /** Snapshot of supplied keys, or null when keys were not supplied. */ | ||
| val flagsChanged: List<String>? = flagsChanged?.toList() |
There was a problem hiding this comment.
For a configuration with multiple keys, Kotlin's toList() produces a mutable ArrayList on the JVM, so a Java listener can modify the value returned by getFlagsChanged() (and Kotlin code can mutate it after a cast). Because DatadogFlagsClient retains and reuses the same FlagsClientEvent for every registration, one listener clearing or removing keys corrupts the event observed by all later subscribers, contrary to the read-only contract. Wrap the snapshot in an actually unmodifiable list or return a fresh defensive copy from the accessor.
Useful? React with 👍 / 👎.
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: waiting
and removed
mergequeue-status: removed
labels
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
What changed
Adds
FlagsClient.onFirstFlags(listener): FlagsSubscriptionso applications can register on an existing client and learn when its first flags are installed from disk or the network, including before the first call tosetEvaluationContext.Each registration receives one retained
FlagsClientEventwithtype = CONFIGURATION_CHANGEDandflagsChanged, all keys from that first accepted configuration, not all flags defined on the server. A valid empty configuration delivers an empty key list. Cache misses, invalid cache and failed fetches do not complete the signal. Late registrations receive the same retained event immediately from memory, without SDK I/O. The event retains the first keys, not an assignment snapshot: evaluations always read the client's current flags.The Kotlin example app registers on its constructed client before creating its OpenFeature provider:
The sample helper logs the supplied keys and evaluates
my-flag-keydirectly with afalsedefault:For shorter-lived owners, retain the returned
FlagsSubscriptionand callunsubscribe()during cleanup. Cancellation is thread-safe, idempotent and local to that registration. It removes pending captures; an already-claimed callback may still run. It does not interrupt callbacks or undo synchronous replay.Pending callbacks run on a dedicated background worker. Available results replay synchronously on the registering thread before
onFirstFlagsreturns, so a later registration can run before an earlier queued one. Callbacks run outside internal locks. Callback Exceptions are logged and isolated; Errors are not caught. Dispatch UI work to the main thread.Registration is directly on the public
FlagsClientinterface. Custom implementations implement this method, and Kotlin interface delegation forwards it. The event listener and subscription remain named functional interfaces; no general event bus is introduced. Events are constructed internally by the SDK; no public event constructor or builder is exposed.Malformed network responses also change behavior for callers that do not subscribe. Previously, parse failure installed an empty configuration and completed the context update successfully. It now follows the failed-fetch path: installed flags and their context are retained, and the update reports failure. Valid empty responses still install successfully. This prevents malformed input from consuming the first-flags signal; it does not introduce a new policy for retaining flags after failed context updates.
Structure and signal flow
Before — existing installation flow
flowchart TB C["DatadogFlagsClient"] E["EvaluationsManager"] N["PrecomputedAssignmentsDownloader<br/>via PrecomputedAssignmentsReader"] R["DefaultFlagsRepository<br/>implements FlagsRepository<br/>Atomic flags and context"] P["FlagsPersistenceManager"] D["DataStoreHandler"] C -->|"setEvaluationContext"| E C -->|"Read current flags and context"| R E -->|"Fetch"| N N -->|"Response parsed by PrecomputeMapper"| E E -->|"setFlagsAndContext"| R R -->|"Construct and save network flags"| P P -->|"Load callback: install only if empty"| R P -->|"Read and write"| D D -->|"Read and write completion"| PAfter — direct registration and first-flags delivery
flowchart TB A["Application"] C["DatadogFlagsClient<br/>implements FlagsClient.onFirstFlags<br/>Retains first FlagsClientEvent"] E["EvaluationsManager"] N["PrecomputedAssignmentsDownloader<br/>via PrecomputedAssignmentsReader"] R["DefaultFlagsRepository<br/>implements FlagsRepository<br/>Current flags and context"] P["FlagsPersistenceManager"] D["DataStoreHandler"] L["FirstFlagsLatch<br/>Retains first installed keys<br/>Owns pending registrations"] W["One-shot ExecutorService<br/>flags-first-flags"] A -->|"onFirstFlags(listener)"| C C -->|"Return FlagsSubscription"| A C -->|"firstFlags.whenComplete"| L A -.->|"unsubscribe: clear this pending registration"| L C -->|"setEvaluationContext"| E C -->|"Evaluate current flags"| R E -->|"Fetch"| N N -->|"Response parsed by PrecomputeMapper"| E E -->|"setFlagsAndContext(context, flags, onInstalled)"| R R -->|"Submit save after network installation"| P P -->|"Cache load: compare-and-set only if empty"| R P -->|"Read and write"| D D -->|"Read and write completion"| P R -.->|"Network: onInstalled settles bookkeeping and context callback"| E R -.->|"Complete after first accepted install and network hook"| L L -.->|"Pending batch"| W W -.->|"Claim listener and deliver keys outside lock"| C L -.->|"Available result: synchronous replay on caller"| C C -.->|"Build or reuse first event; invoke listener outside lock"| AThe repository continues to own persistence and its separate persistence-load barrier. A cached configuration completes the first-flags latch only if its compare-and-set installation succeeds. For a first network installation, the repository submits persistence, runs
onInstalledto settle initialization bookkeeping and context completion, then completes the latch infinally. Disk write completion is not awaited, and ordinary storage-submission exceptions are logged without aborting an accepted installation.The latch keeps the first installed keys even when the repository later replaces its current flags. The client constructs and retains one event from those keys. Pending delivery uses a one-shot worker; late replay uses the caller thread. Cancellation can suppress a queued listener until it is claimed for delivery.
Why
Cached flags can be available before network initialization finishes. Applications need a reliable signal that flags have been installed so they can evaluate them directly. Registering on an existing client avoids constructor callback timing problems, and retaining the first result prevents fast disk or network completion from being missed.
This notification is independent of readiness and does not change OpenFeature evaluation gates or evaluation reasons. Tracks FFLSDK-254.
The manager-level regression for successful installation after initialization timeout remains included. Client integration coverage is deferred to reliability/single-fit or RUM FIT/FLEX, including Flags module support as needed.