chore: improvements to ETP#4586
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files
🚀 New features to boost your workflow:
|
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: e4cd6de | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback! |
BenchmarksBenchmark execution time: 2026-04-14 09:00:07 Comparing candidate commit e4cd6de in PR branch Found 15 performance improvements and 0 performance regressions! Performance is the same for 203 metrics, 6 unstable metrics.
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
darccio
left a comment
There was a problem hiding this comment.
In general it's fine, but the regressions must be addressed.
I'm curious why we need to set _dd.span_links when we could just postpone the serialization until the very moment we really need to.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c65531fd5d
ℹ️ 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".
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
| //go:noinline | ||
| func warnUnsupportedValue(t uint32) { | ||
| log.Warn("failed to serialize unsupported type: %d", t) | ||
| } |
There was a problem hiding this comment.
Why does it need the go:inline directive?
There was a problem hiding this comment.
:hmmnod: It's not strictly necessary here, I think, since the callers are already pretty complex, so there's no chance of inlining unless we simplify the them. But that might be difficult...
ddtrace/tracer/payload_v1.go:941:6: cannot inline anyValue.encode: function too complex: cost 1059 exceeds budget 80
ddtrace/tracer/payload_v1.go:713:6: cannot inline (*payloadV1).encodeSpanEvents: function too complex: cost 1517 exceeds budget 80
So, tl;dr, probably doesn't need go:noinline
What does this PR do?
stringkeys instead ofstringValuein the indices map to reduce map lookup timesMotivation
Align with RFC and Java implementation. Improvements to performance.
Reviewer's Checklist
make lintlocally.make testlocally.make generatelocally.make fix-moduleslocally.Unsure? Have a question? Request a review!