Repository navigation
FFLSDK-272: Send additional Flagging state telemetry on flag requests - #4030
Conversation
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. |
![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: 05fcdef98b
ℹ️ 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".
![datadog-prod-us1-6[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
The diagnostics preserve existing assignment data while maintaining a stable per-client identity and synchronized fetch sequence. The completed static review identified no actionable defect, but focused tests could not execute because a required aapt2 dependency was unavailable in the offline cache.
🤖 Bits Code Review · Commit 05fcdef
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 24b2e0b | Docs | View more details | Give us feedback! |
vjfridge
left a comment
vjfridge
left a comment
There was a problem hiding this comment.
Looks like we have merge conflict to resolve and I left a couple non-blocking comments but otherwise looks good to me!
61f6dce to
6cfb11c
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Deployed edge/proxy acceptance of the new payload and the prerequisite EVP schema remain unverified.
2 open issues
What changed in this PR
Adds per-instance and per-fetch diagnostics to Flags assignment requests, helping distinguish SDK clients and logical fetches when investigating repeated requests.
Changes:
- Generates a UUIDv7 instance ID and captures fetch sequence and trigger in an immutable snapshot.
- Passes diagnostics through the downloader and serializes them into the request body.
- Adds identity, sequencing, concurrency, UUID format, and request-body tests.
| File | Description |
|---|---|
features/dd-sdk-android-flags/src/test/kotlin/com/datadog/android/flags/utils/forge/ForgeConfigurator.kt |
Registers the diagnostics fixture factory. |
features/dd-sdk-android-flags/src/test/kotlin/com/datadog/android/flags/utils/forge/FlagsRequestDiagnosticsForgeryFactory.kt |
Generates diagnostics test fixtures. |
features/dd-sdk-android-flags/src/test/kotlin/com/datadog/android/flags/internal/net/PrecomputedAssignmentsRequestFactoryTest.kt |
Verifies diagnostic fields and unchanged request content. |
features/dd-sdk-android-flags/src/test/kotlin/com/datadog/android/flags/internal/evaluation/EvaluationsManagerTest.kt |
Tests instance identity and logical fetch sequencing. |
features/dd-sdk-android-flags/src/test/kotlin/com/datadog/android/flags/internal/diagnostics/UUIDv7GeneratorTest.kt |
Checks UUID timestamp, version, variant, and uniqueness. |
features/dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/internal/net/PrecomputedAssignmentsRequestFactory.kt |
Adds optional diagnostics to request JSON. |
features/dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/internal/net/PrecomputedAssignmentsReader.kt |
Accepts a diagnostics snapshot. |
features/dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/internal/net/PrecomputedAssignmentsDownloader.kt |
Forwards diagnostics to the request factory. |
features/dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/internal/evaluation/EvaluationsManager.kt |
Owns instance identity and allocates fetch diagnostics. |
features/dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/internal/diagnostics/FlagsRequestDiagnostics.kt |
Defines diagnostics, triggers, and UUIDv7 generation. |
detekt_custom_unsafe_calls.yml |
Registers the JSON map constructor as potentially throwing. |
Review effort: Balanced
6cfb11c to
b29a633
Compare
| } | ||
| } | ||
|
|
||
| /** UUIDv7 polyfill for supported Android runtimes without a UUIDv7 factory. */ |
There was a problem hiding this comment.
so in fact for every Android? because UUID v7 is not part of Android API https://developer.android.com/reference/java/util/UUID to my knowledge, and in Kotlin it is available only starting from Kotlin 2.3.
There was a problem hiding this comment.
Simplified the KDoc in beeb6aad to “UUIDv7 Generator.” and retained the RFC 9562 bit-layout comment. The period is required by the repository’s KDoc lint rule.
🤖written by a bot, edited by a human
| private val initializationTimeoutMs: Long?, | ||
| private val initializationTimeoutScheduler: InitializationTimeoutScheduler | ||
| ) { | ||
| private val instanceId = UUIDv7Generator.generate(sdkCore.timeProvider.getDeviceTimestampMillis()).toString() |
There was a problem hiding this comment.
Is UUID v7 really needed here? Maybe simply UUID v4 will work?
There was a problem hiding this comment.
Retained UUIDv7 to give a hint of relative SDK instance creation time for correlation with other client instances. The implementation in beeb6aad uses UUID.randomUUID() plus timestamp/version bits, without another dependency or timestamp suffix.
🤖written by a bot, edited by a human
| callback: EvaluationContextCallback? = null, | ||
| onFetchFinished: () -> Unit = {} | ||
| ) { | ||
| val diagnostics = synchronized(fetchSequenceLock) { |
There was a problem hiding this comment.
side note: I'm quite concerned by the growing amount of synchronized + AtomicXXX in the flags module code. Maybe it is needed to revisit threading strategy and adopt a single processing thread.
There was a problem hiding this comment.
Could you please expand on the specifics of the concerns? I am not disagreeing with the risk brought by the complexity of all the locks-of-various-kinds - I just want to learn more about the concerns and processing strategies available here.
There was a problem hiding this comment.
It is really hard to keep the threading model of that in head and I think, depending on the calls we use, we also increase the risk of thread being parked for too long, waiting for the lock to be acquired.
There was a problem hiding this comment.
appreciate the explanation, Nikita.
I think a Single-threaded worker queue is actually off the table because of the interplay between various callbacks and i/o work. There is work that can be done in parallel and work that needs to block other work; the strategies now are fine-grained to support the throughput.
Do you have any examples of modules where an easier-to-understand threading model is able to handle to variety of mutating actions and states-with-contstraints we need in the flagging module? I am looking for inspiration to help strike the balance between readability and throughput of the flagging SDK's API.
There was a problem hiding this comment.
I think a Single-threaded worker queue is actually off the table because of the interplay between various callbacks and i/o work.
We use it in RUM. Having a single thread reduces the risk of concurrency bugs for a given pipeline.
Speaking of I/O work - at least when it is time to write to the disk (all the business logic was already applied), there withWriteContext provides EventWriteScope anyway which is executed on I/O thread.
There was a problem hiding this comment.
Thanks. That's a really helpful direction to explore foe Flagging.
For now at least, I've simplified the sequence ID to just an atomic long. The sequence identifier is best effort and still useful if its issuance is not precisely deterministic (first to call vs first to send etc.)
There was a problem hiding this comment.
maybe simple UUID v4 instead or UUID v7 will work as well.
There was a problem hiding this comment.
We do want to keep some semblance of ordering though so v4 is out. The timestamp in v7 would be subject to the same timing indeterminism as a monotonically increasing natural number so I don't think a uuid is beneficial here.
| @ParameterizedTest | ||
| @EnumSource(FlagsRequestDiagnostics.Trigger::class) |
There was a problem hiding this comment.
I would discourage using @ParameterizedTest. It seems flags module started to use it quite a lot, but I think it is better simply to use a forged value and run test once - this way we also save test execution time.
There was a problem hiding this comment.
Replaced trigger parameterization in beeb6aad with a forged diagnostics snapshot. The assertion still uses the explicit wire strings.
🤖written by a bot, edited by a human
| override fun getForgery(forge: Forge): FlagsRequestDiagnostics = FlagsRequestDiagnostics( | ||
| instanceId = forge.getForgery<UUID>().toString(), | ||
| sequence = forge.aLong(min = 1), | ||
| trigger = forge.aValueFrom(FlagsRequestDiagnostics.Trigger::class.java) |
There was a problem hiding this comment.
nit: since it is enum, it can be simply forge.getForgery(). type inference will do the job.
There was a problem hiding this comment.
Changed the trigger forgery in beeb6aad to forge.getForgery(), using type inference.
🤖written by a bot, edited by a human
| // Then | ||
| assertThat(finished).isEqualTo(1) | ||
| verify(mockAssignmentsDownloader, never()).readPrecomputedFlags(any(), any()) | ||
| verify(mockAssignmentsDownloader, never()).readPrecomputedFlags(any(), any(), any()) |
There was a problem hiding this comment.
Changed the unavailable-context test in beeb6aad to verify no downloader interactions at all, so unexpected calls with different arguments cannot pass.
🤖written by a bot, edited by a human
| callback: EvaluationContextCallback? = null, | ||
| onFetchFinished: () -> Unit = {} | ||
| ) { | ||
| val diagnostics = synchronized(fetchSequenceLock) { |
b29a633 to
beeb6aa
Compare
There was a problem hiding this comment.
More details
Fetch sequences are intentionally best-effort identifiers under concurrency, without a guarantee of network dispatch order. Deployed edge/proxy acceptance and EVP schema readiness remain unverified release prerequisites.
🤖 Bits Code Review · Commit beeb6aa
beeb6aa to
b5fd325
Compare
Identify SDK instances with UUIDv7 and logical fetches with an atomic sequence and trigger. Pass generated diagnostics models through the fetch path and serialize the request once.
b5fd325 to
24b2e0b
Compare
The base branch was changed.
There was a problem hiding this comment.
More details
Per-fetch diagnostics retain a stable client-instance identifier and atomic sequence through request serialization. The completed static review found no actionable defects; human discussion confirms sequence ordering is intentionally best effort.
🤖 Bits Code Review · Commit 24b2e0b
What does this PR do?
References FFLSDK-272.
Send 3 new fields in the precomputed assignments POST:
sdk.instance_id- uniquely identifies the in-memory configuration-owning client instancefetch.sequence- a logical fetch operation in the SDK's lifecyclefetch.trigger- the SDK's internal accounting of what triggered the fetchMotivation
Distinguish configuration-owning SDK instances and logical fetches when investigating repeated assignment requests in edge request logs.
Implementation
EvaluationsManagerowns the UUIDv7 instance ID and fetch sequence. Each accepted fetch captures an immutable diagnostics value, which travels with that fetch to the existing request factory. The factory adds its fields directly to the structured request body and serializes the complete body once.The in-package
UUIDv7Generatorsupplies the instance ID. A diagnostics trigger enum usesinitializationandcontext_change, consistently across SDKs.Additional Notes
Local validation: all 613 Flags unit tests, Detekt, ktlint and API checks passed. The API check used the unchanged generated reference with apiDump excluded to avoid the existing Gradle task-ordering conflict. Tests cover instance identity, logical sequencing, concurrent acceptance and the complete request body. Verify deployed edge/proxy acceptance and the EVP schema prerequisite before SDK release.
Review checklist (to be filled by reviewers)
FFE reviewer selection
Reviewers: @sameerank, @leoromanovsky
Why: android — Android Flags SDK maintenance. Reviewers come from the matched pod.
🤖 Automated selection.
Selection details
Initiative: fallback / android
Why this match: Android Flags SDK maintenance. The change adds a diagnostics object (a UUIDv7 SDK instance ID, a logical fetch sequence, and an initialization/context_change trigger) to the precomputed-assignments request body. Its purpose is to correlate repeated assignment requests in edge request logs. This is operational request instrumentation, not onboarding/readiness diagnostics (guided-onboarding), evaluation telemetry over EVP (flag-evaluation-evp), or any other registered project's behavior, so it falls back to the Android subject area.
The PR author was excluded. Existing reviewers and teams were preserved.
Debug or fix a problem
Workflow run · Bits Code session
dry_run=false. Unchanged completed runs are skipped. If the receipt saysneeds_reconciliationor a write outcome is uncertain, ask the workflow owner to reconcile GitHub and the receipt before retrying; do not delete the lock blindly.Evidence and run details
✅ Complete — reviewer coverage verified. The workflow checked pending requests or submitted reviews on the evaluated head.
Generated asynchronously by the FFE reviewer-routing workflow.
Changed paths:
features/dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/internal/diagnostics/FlagsRequestDiagnostics.kt,features/dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/internal/evaluation/EvaluationsManager.kt,features/dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/internal/net/PrecomputedAssignmentsRequestFactory.kt,features/dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/internal/net/PrecomputedAssignmentsDownloader.kt,features/dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/internal/net/PrecomputedAssignmentsReader.ktEvaluated head:
b29a63304c2e103f89dc2703fd7e267c2c121503Registry revision:
revision:1946aa2a-530e-487a-84a7-83aff44a1089Pinned skill source:
d214cfc65ae154b97d39af5cf32caf48aaec9bf0Instructions: workflow-injected snapshot (no plugin loaded)
Instruction snapshot SHA-256:
f23b2a88c13c0da90e2dd58246a14f257af91e2195c555039bd555b52e6f3516