feat(openfeature): add span enrichment#4844
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39968c94bd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 2e59473 | Docs | Datadog PR Page | Give us feedback! |
Codecov Report❌ Patch coverage is Additional details and impacted files
🚀 New features to boost your workflow:
|
05a6a4c to
980c094
Compare
BenchmarksBenchmark execution time: 2026-06-25 02:11:42 Comparing candidate commit 2e59473 in PR branch Found 2 performance improvements and 1 performance regressions! Performance is the same for 294 metrics, 2 unstable metrics, 1 flaky benchmarks without significant changes.
|
980c094 to
4c3c5dc
Compare
sameerank
left a comment
There was a problem hiding this comment.
A couple bits of feedback. Let me know if you need help with checking these changes against the tests in DataDog/system-tests#6828 There should be a way to run it locally
858ebd6 to
0e2b1ba
Compare
sameerank
left a comment
There was a problem hiding this comment.
Thanks for making the requested changes!
| func (se *SpanEnrichment) addSerialID(sid uint32) { | ||
| if _, exists := se.serialIDs[sid]; !exists { | ||
| if len(se.serialIDs) >= spanEnrichmentMaxSerialIDs { | ||
| log.Debug("openfeature: span enrichment: too many flag serial IDs, dropping (max %d)", spanEnrichmentMaxSerialIDs) |
There was a problem hiding this comment.
Should we have telemetry on these drops?
There was a problem hiding this comment.
Yeah, I think it's useful. Dropping serial ids may impact experiment results, so worth flagging. I'm going to add it in a follow-up — looks like I need to add the new metric in dd-go first, also not sure if triggering telemetry while holding span mutex is safe
5c99705 to
e5e23b0
Compare
e5e23b0 to
2465a86
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2465a86288
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| eval.Subject = evalCtx.TargetingKey() | ||
| } | ||
| return eval | ||
| } else if evalDetails.Variant == "" { |
There was a problem hiding this comment.
Treat type mismatches as runtime defaults
When a matched flag carries a variant/serialId but the caller requests an incompatible type, the SDK returns the caller's default with TypeMismatchCode while preserving the provider's variant metadata; the existing flag-evaluation path treats that case as a runtime default. This check only handles Variant == "" and the serial-id branch above wins first, so span enrichment records the experiment serial/subject (or drops the default if no serial is present) even though the application received the default value, causing traces to be associated with variants users did not receive.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Erronous evaluations must not set serial id/variant
There was a problem hiding this comment.
I noticed this as well in my Java implementation; the evaluator checks type compatibility before it attaches variant metadata / serial ID metadata. I believe there is an existing bug in the Go implementation.
What Java does have is safer because it checks type compatibility first and returns TYPE_MISMATCH.
Then the evaluator only attaches __dd_split_serial_id after that check passes.
Finally, SpanEnrichmentHook.java can safely prefer serial ID because Java never leaves serial metadata on a type-mismatch result.
2465a86 to
2e59473
Compare
leoromanovsky
left a comment
There was a problem hiding this comment.
One risk identified is that it can emit ffe_flags_enc for a variant the app did not actually receive.
I added a stack PR with a new test to demonstrate: #4953
| evalLoggingHook = newFlagEvalLoggingHook(evalWriter) | ||
| } | ||
|
|
||
| var spanEnrichmentHook *spanEnrichmentHook |
There was a problem hiding this comment.
Oh, shutdown, are you able to clean up the span enrichment state?
There was a problem hiding this comment.
I don't think there's a need because the state is tied to spans (and in particular to root spans). So when those spans are finished, it's going to send and cleanup that state. There's no state in span enrichment hook itself
| eval.Subject = evalCtx.TargetingKey() | ||
| } | ||
| return eval | ||
| } else if evalDetails.Variant == "" { |
There was a problem hiding this comment.
I noticed this as well in my Java implementation; the evaluator checks type compatibility before it attaches variant metadata / serial ID metadata. I believe there is an existing bug in the Go implementation.
What Java does have is safer because it checks type compatibility first and returns TYPE_MISMATCH.
Then the evaluator only attaches __dd_split_serial_id after that check passes.
Finally, SpanEnrichmentHook.java can safely prefer serial ID because Java never leaves serial metadata on a type-mismatch result.
leoromanovsky
left a comment
There was a problem hiding this comment.
Let's discuss plans as a team about resolving (#4953) with a cohesive effort.
|
/merge |
|
View all feedbacks in Devflow UI.
This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
devflow unqueued this merge request: It did not become mergeable within the expected time |
<!-- * New contributors are highly encouraged to read our [CONTRIBUTING](/CONTRIBUTING.md) documentation. * Commit and PR titles should be prefixed with the general area of the pull request's change. --> ### What does this PR do? <!-- * A brief description of the change being made with this pull request. * If the description here cannot be expressed in a succinct form, consider opening multiple pull requests instead of a single one. --> Add an optional integration between openfeature and tracer that adds feature flag evaluation details as span tags. These are later decoded by the backend to associate traces with experiments. [More details in RFC](https://docs.google.com/document/d/1rtO4uCZ0-y5iLfNkKjIbHqWpx2qF4cHZV13ENafuomk/edit?tab=t.0#). ### Motivation <!-- * What inspired you to submit this pull request? * Link any related GitHub issues or PRs here. * If this resolves a GitHub issue, include "Fixes #XXXX" to link the issue and auto-close it on merge. --> This deepens integration between FFE (feature flagging and experimentation) and APM, and allows users to run experiments on APM-generated metrics. ### Reviewer's Checklist <!-- * Authors can use this list as a reference to ensure that there are no problems during the review but the signing off is to be done by the reviewer(s). --> - [ ] Changed code has unit tests for its functionality at or near 100% coverage. - [X] [System-Tests](https://github.com/DataDog/system-tests/) covering this feature have been added and enabled with the va.b.c-dev version tag. DataDog/system-tests#7125 - [ ] There is a benchmark for any new code, or changes to existing code. - [X] If this interacts with the agent in a new way, a system test has been added. - [X] New code is free of linting errors. You can check this by running `make lint` locally. - [X] New code doesn't break existing tests. You can check this by running `make test` locally. - [X] Add an appropriate team label so this PR gets put in the right place for the release notes. - [X] All generated files are up to date. You can check this by running `make generate` locally. - [ ] Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @/DataDog/dd-trace-go-guild. Make sure all nested modules are up to date by running `make fix-modules` locally. Unsure? Have a question? Request a review! Co-authored-by: benjamin.debernardi <benjamin.debernardi@datadoghq.com>
What does this PR do?
Add an optional integration between openfeature and tracer that adds feature flag evaluation details as span tags. These are later decoded by the backend to associate traces with experiments. More details in RFC.
Motivation
This deepens integration between FFE (feature flagging and experimentation) and APM, and allows users to run experiments on APM-generated metrics.
Reviewer's Checklist
make lintlocally.make testlocally.make generatelocally.make fix-moduleslocally.Unsure? Have a question? Request a review!