Repository navigation
fix(ffe): attribute flag evaluations to the connection application - #2716
Conversation
Snapshot service, environment, and application version before coalescing empty-context batches. Preserve explicit context, request lifecycle ordering, and privacy rules. FFL-3438 Co-authored-by: Codex <noreply@openai.com>
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 1db6705 | Docs | View more details | Give us feedback! |
BenchmarksComparisonCandidateCandidate benchmark detailsBaselineBaseline benchmark details |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (1)ℹ️ 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 Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: 58b738075f
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
There was a problem hiding this comment.
🟢 Approval recommended
The narrowly scoped attribution change preserves explicit context and privacy behavior, with focused regression coverage and no unresolved findings.
No open issues 🎉
What changed in this PR
This PR attributes empty-context flag evaluations to the sidecar connection’s current application before aggregation, preventing evaluations from different applications from sharing attribution.
Changes:
- Snapshot application identity and fill missing event-level service values while preserving explicit context and degraded-event privacy.
- Add regression coverage for application changes, separate connections, clearing, reconnect replay, and privacy.
- Document attribution behavior and application-update ordering.
| File | Description |
|---|---|
datadog-sidecar/src/service/sidecar_server.rs |
Apply attribution before coalescing and add identity and ordering regression tests. |
datadog-sidecar/src/service/mod.rs |
Document attribution rules for flag-evaluation submissions. |
datadog-sidecar/src/service/flagevaluation_privacy_tests.rs |
Verify application attribution and preserved privacy through IPC-to-HTTP delivery. |
Review effort: Balanced
typotter
left a comment
typotter
left a comment
There was a problem hiding this comment.
LGTM pending addressing codex's security finding.
|
/code blockers |
|
View all feedbacks in Devflow UI.
Checking merge blockers for #2716...
Detected 3 merge blocker(s) to address: 🟠 Pending
|
aarsilv
left a comment
aarsilv
left a comment
There was a problem hiding this comment.
Thanks for iterating! Just a few remaining nits you can take or leave as you see fit! 🚀
yannham
left a comment
yannham
left a comment
There was a problem hiding this comment.
Nothing to say from APM common component side 👍
typotter
left a comment
typotter
left a comment
There was a problem hiding this comment.
approved - nothing new to add on top of "Aaron"
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
…valuation-application-identity # Conflicts: # datadog-sidecar/src/service/sidecar_server.rs
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: queued
mergequeue-status: in_progress
and removed
mergequeue-status: queued
labels
What does this PR do?
Attach application identity to
flagevaluationtrack EVP events in the sidecar, using the application already registered on the connection.Motivation
PHP #4288 intentionally sends empty application context instead of repeating service, environment, and application version on every evaluation. Until the sidecar fills those values, evaluations lack application attribution and different applications can share aggregation buckets.
This implements FFL-3438, following the application-identity review discussion.
Changes and key decisions
context.dd.serviceon non-degraded events. Keep explicit event-level service values, and never add per-event environment/version or restore context to degraded events.enqueue_actions_for_servicedo not inherit the connection's application identity.Blast radius
flagevaluationsubmissions with a registered application.How to test the change?
Latest native validation (
2703c8ac8): 174 sidecar tests pass on Rust 1.91.1 / Linux ARM64; compilation, formatting, strict Clippy, and structural lint checks pass. macOS/Windows runtime coverage remains with CI.Regression coverage: application changes before flush, separate connections, request-end clearing, explicit context, pending application replay, 256/257-character boundaries including Unicode, and the service-routing exclusion. Real IPC → HTTP tests check attribution, protected/consented/degraded payloads, raw PII canaries, and unchanged SDK-origin headers.
C interface checks at 06e79521e: 220 sidecar/FFI tests passed with one existing skip, four doctests passed, and 13 C/C++ examples passed with two runner-defined skips. Generated C headers include both submission functions' attribution documentation.
Earlier PHP/dogfooding validation: PHP #4368 used native
58b7380and PHPe9e88caf2, before the later native fixes. On Linux ARM64 / PHP 8.2 NTS, 23 PHPT tests passed with three environment-dependent skips; the two-worker request-reuse probe attributed 8/8 evaluations correctly. Dogfooding matched master with 36/36 successful evaluations and 36 OTel metrics, unchanged results, and correct application service tags. Live configuration enabled full consent.Earlier system-tests on those same revisions: 51/53 passed versus 48/53 on master, including all three consent modes. Remaining failures were the context-bounds fixture rejecting the service tag (FFL-3407) and shared-sender backpressure during the high-volume count test on both builds. These results do not validate the latest native revision or establish equivalent stress performance.
Before releasing the PHP functionality, update PHP #4368 to the merged libdatadog version and rerun PHP validation, system-tests, and ffe-dogfooding (FFL-3439).