Repository navigation
Add local flag evaluation telemetry opt-out - #3990
leoromanovsky wants to merge 1 commit 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: 382669c | 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-datadog-prod-us1-2[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
The new flags integration suite is absent from default CI, leaving its strict-type and local-update checks without automated regression coverage. No functional defect in the telemetry control was identified by the static reviewer.
🤖 Bits Code Review · Commit 382669c
| // SINGLE FEATURE INTEGRATION TESTS | ||
| include(":reliability:single-fit:logs") | ||
| include(":reliability:single-fit:rum") | ||
| include(":reliability:single-fit:flags") |
There was a problem hiding this comment.
Registering the flags module does not make its integration tests run in default CI. The general test jobs exclude reliability modules, and the single-fit jobs invoke each existing suite explicitly. Consequently, the new strict-type and local-update checks provide no automated regression protection. Add a job in ci/pipelines/default-pipeline.yml invoking :reliability:single-fit🎏testReleaseUnitTest.
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: 382669c27f
ℹ️ 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".
| // SINGLE FEATURE INTEGRATION TESTS | ||
| include(":reliability:single-fit:logs") | ||
| include(":reliability:single-fit:rum") | ||
| include(":reliability:single-fit:flags") |
There was a problem hiding this comment.
Register this suite in CI as well. I checked ci/pipelines/default-pipeline.yml: its single-FIT jobs explicitly run Logs, RUM, Trace, OkHttp, and Profiling, while the regular verification jobs are scoped to core, features, and integrations, so including this project here never schedules :reliability:single-fit:flags:testReleaseUnitTest. The new end-to-end coverage can therefore regress without any PR signal; add the corresponding flags test-pyramid job.
AGENTS.md reference: AGENTS.md:L196-L200
Useful? React with 👍 / 👎.
satween
left a comment
satween
left a comment
There was a problem hiding this comment.
Please consider make tests smaller and simplify the complexity of the configuration.
Otherwise it's becomes very hard to verify that tests actually verifies that logic is correct, making AI do whatever it wants and loosing control of the implementation 🤖
| flagsClient.setEvaluationContextSuspend(newContext.toDatadogEvaluationContext()) | ||
| updateEvaluationTelemetryPolicy(newContext) | ||
| val newTargetingContext = targetingContext(newContext) | ||
| val controlChanged = oldContext?.asMap()?.get(EVALUATION_TELEMETRY_OPT_OUT) != |
There was a problem hiding this comment.
I see that many operations requires asMap call. Maybe worth to support get extension method for EvaluationContext ? That would simplify further migration if public API of openfeature api would be changed
| provider.initialize(original) | ||
|
|
||
| // When | ||
| runtime.server.enqueue(MockResponse().setResponseCode(500)) |
There was a problem hiding this comment.
This block is very complex for the WHEN - consider split this test into several that checks the specific scenario, or, at least move the logic into self-describable method, to make it easier to understand what scenario is being tested
| it.getAsJsonObject("feature_flags")?.get("manual")?.asString == "preserved" | ||
| } | ||
| ).isTrue() | ||
| client.setEvaluationTelemetryOptOut(false) |
There was a problem hiding this comment.
Could we keep the actions in When and move the assertions to Then? This test currently checks results during setup and performs another policy change after Then. Separating these steps would make it easier to see what triggers the behavior and which results we expect. The telemetry re-enablement could become a separate test.
| } | ||
|
|
||
| @Test | ||
| fun `M retain values and exposures W native opt out and snapshots`(forge: Forge) { |
There was a problem hiding this comment.
Could we split this into smaller tests for the independent behaviors? It currently covers value types, error cases, diagnostics, snapshot tracking, manual RUM annotations, and telemetry re-enablement. Separate tests would make failures easier to diagnose and allow the remaining scenarios to run when one fails. Assertions that describe the same behavior can still stay together.
| } | ||
|
|
||
| @Test | ||
| fun `M suppress only strict Boolean true W provider initialize`(forge: Forge) = runBlocking<Unit> { |
There was a problem hiding this comment.
Could we name the method exercised in When and describe initialization as the precondition? For example: M suppress evaluation telemetry W getBooleanEvaluation() {initialized with Boolean true opt-out}. Currently, the name points to initialize(), but that call happens in Given. Matching the name to the action makes the test report easier to understand without opening the implementation.
|
|
||
| @ParameterizedTest | ||
| @ValueSource(booleans = [false, true]) | ||
| fun `M gate evaluation telemetry independently W setEvaluationTelemetryOptOut`(enabled: Boolean, forge: Forge) { |
There was a problem hiding this comment.
Could we include resolve() in the test name? The behavior being checked happens when a flag is resolved after changing the policy. Something like W setEvaluationTelemetryOptOut(true) + resolve() would make that sequence explicit. Alternatively, we could move the policy setup into Given and name resolve() as the action.
| val stubCore = StubSDKCore(forge, app) | ||
| private val store = MemoryStore() | ||
| private val flushTasks = mutableListOf<Runnable>() | ||
| private val scheduler = mock<ScheduledExecutorService>() |
There was a problem hiding this comment.
Small naming suggestion: could we use mockApp and mockScheduler for the Mockito mocks, and fakeControl and fakeContext for the test inputs in ConvertersTest? These prefixes help distinguish mocked dependencies from concrete test data when reading the setup.
| internal class ConvertersTest { | ||
|
|
||
| @Test | ||
| fun `M exclude local control W toDatadogEvaluationContext`(forge: Forge) { |
There was a problem hiding this comment.
Could we inject these strings with @StringForgery parameters instead of passing Forge and generating them inside the test? Since these are independent string inputs, parameter annotations would make them visible in the test signature and keep the setup focused on building the context. We can preserve the alphabetic constraint with a regex where needed.
| Value.String("false"), Value.Integer(1), Value.Double(1.0), Value.List(listOf(Value.Boolean(true))), | ||
| Value.Structure(mapOf("value" to Value.Boolean(true))) | ||
| ) | ||
| for (control in controls) { |
There was a problem hiding this comment.
Could we turn these control values into a @ParameterizedTest with a @MethodSource? Each value represents an independent case. Parameterization would give each one its own result in the test report and ensure that a failure for one value does not prevent the others from being checked.
4 participants
Motivation
Applications need to apply customer-owned policies to evaluation telemetry without changing flag results or experiment exposures.
Changes and Decisions
dd_evaluation_telemetry_opt_out: truebefore attribute conversion.FlagsClientinterface. The extension avoids adding an abstract method to customer implementations.Validation
The real library integration harness used local assignment HTTP, actual evaluation/exposure writers, and the RUM feature. Debug and release runs covered strict types, local updates, pending and failed requests, cached/snapshot values, independent clients, and manual RUM annotations.
The baseline emitted six evaluations with the opt-out attribute. The implemented control suppresses those evaluations while preserving exposures and flag results. This is JVM library runtime evidence, not an emulator or staging deployment.