fix(profiling): resolve web tags for descendants cached before promotion - #9805
fix(profiling): resolve web tags for descendants cached before promotion#9805szegedi wants to merge 1 commit into
Conversation
Overall package sizeSelf size: 8.13 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 3.3.3 | 125.43 kB | 441.68 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | dc-polyfill | 0.1.11 | 25.74 kB | 25.74 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
![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: c2b9093ea2
ℹ️ 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".
| cached.webTags = tags | ||
| // This span may be the ancestor a descendant walked past before it counted as | ||
| // a web-server span, so every empty answer computed until now is suspect. | ||
| generation++ |
There was a problem hiding this comment.
With endpoint collection or the OTEP thread-context writer enabled, every HTTP request span is first cached as a miss and then promoted, so this process-global generation++ makes misses in unrelated traces stale. A long-lived non-web child span that is activated after each web request will repeatedly re-walk its parent chain even though only another trace changed, and the new wall/OTEP recheck calls this from storage-enter hot paths; please keep the generation per trace or otherwise invalidate only the affected ancestry.
AGENTS.md reference: AGENTS.md:L194-L199
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fair, I changed it now so that the counter now lives on the trace (a Symbol on _trace) rather than in the module.
Per-trace isn't just cheaper, it's the correct scope. Cost is now bounded by promotions in the span's own trace. This is normally one, (the request span itself) instead of one per request process-wide.
I didn't go as far as invalidating only the affected ancestry, which would need a reverse parent-to-descendants index the cache doesn't have and would have to maintain on the hot path. Per-trace gets essentially all of the benefit for one property read.
The change also surfaced a harness bug: npm run test:profiler went red on the new wall-profiler test, and the cause was actually the test harness. makeChildSpan in wall.spec.js built the child with its own
_trace: { started: [webSpan] }, so parent and child sat in different traces, which is obviously wrong. Fixed to share the parent's _trace; I'm mentioning this for a reviewer that wonders about the change to the test harness.
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 2fa615a | Docs | Datadog PR Page | Give us feedback! |
The shared web-tags cache records "no web-server ancestor" for a span and never revisits it, but that answer expires: plugins set `span.type` after creating the span — TracingPlugin.startSpan activates it before addRequestTags runs — so a child created in that window walks past an ancestor that is about to become a web-server span, and caches a miss for a chain that is about to have one. Promotion can't find those descendants, since the walk only goes upwards. So a promotion now bumps a generation counter, and an empty answer older than the counter is walked again on the next lookup. Resolved answers are untouched (the cached bag is the ancestor's live tag object), and a span with no parent is stamped as permanently empty, since only its own promotion could change it and onTagsUpdate already handles that. When a re-walk turns an empty answer into a real one, the cache publishes resolvedCh for that span, the same announcement a promoted span gets, so a consumer doesn't have to care which way the ancestry appeared. The counter lives on the trace, not in the module. A promotion can only invalidate empty answers within its own trace, because the walk follows `_parentId` through `_trace.started` and never leaves it — while a process-global counter would have every HTTP request's promotion invalidate empty answers in unrelated traces, making a long-lived non-web span re-walk its chain once per request served elsewhere, from the storage-enter path. Invalidation alone fixes nothing, because both consumers only ask the cache while building their per-span state, and the spans this affects already have theirs built. So each now asks again for state built from an empty answer: - the OTEP-4947 writer re-checks on re-entry when a record was built with no web-server ancestor, and attaches the endpoint or enlists the record for the request's endpoint announcement. Guarded so a re-entrant announcement from inside the lookup can't append the endpoint twice. - the wall profiler re-checks in #getProfilingContext when the snapshot it holds has no webTags, since #spanTagsUpdated only fires for a span promoted itself, never for descendants that walked past it beforehand. Both re-checks cost two property reads plus, at most, the cache's own generation compare — a walk only happens when a promotion in that trace has actually invalidated something. Two test-harness inaccuracies are fixed along the way, both of which had been hiding behaviour rather than testing it: web-tags-cache.spec.js's makeSpan returned a fresh object from context() per call, so spying on it counted calls on a throwaway and the "walks the parent chain once" assertion held vacuously; and wall.spec.js's makeChildSpan gave the child its own _trace object, so parent and child were in different traces, which no real trace chunk is. Reported by codex on #9210 and #9805.
c2b9093 to
2fa615a
Compare
BenchmarksBenchmark execution time: 2026-08-13 11:05:17 Comparing candidate commit 2fa615a in PR branch Found 1 performance improvements and 0 performance regressions! Performance is the same for 2310 metrics, 47 unstable metrics.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9805 +/- ##
==========================================
- Coverage 98.53% 98.52% -0.02%
==========================================
Files 972 973 +1
Lines 141744 142099 +355
Branches 12317 12711 +394
==========================================
+ Hits 139669 139997 +328
- Misses 2075 2102 +27 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
What does this PR do?
Makes the shared web-tags cache correct an empty answer once an ancestor becomes a web-server span, and has both consumers pick that up for state they already built.
Motivation
Follow-up to the review of #9210 (discussion).
getCachedWebTagscaches "no web-server ancestor" permanently, but plugins setspan.typeafter creating the span —TracingPlugin.startSpanactivates it beforeaddRequestTagsruns — so a child created in that window walks past an ancestor that is about to become a web-server span and caches a miss for a chain that is about to have one. The ancestor's promotion can't find those descendants, because the walk only goes upwards. The result is a request whose downstream spans permanently lose their endpoint: nodatadog.trace_endpointin the OTEP-4947 record, and notrace endpointlabel on wall-profiler samples.Verified before fixing, with the real cache:
Additional Notes
Every promotion bumps a generation counter; an empty answer older than it is walked again on the next lookup. Resolved answers are never revisited (the cached bag is the ancestor's live object, so later tag changes land in it anyway), and a span with no parent is stamped permanently empty since only its own promotion could change it —
onTagsUpdatealready covers that. A re-walk that resolves publishesresolvedChfor that span, the same announcement a promoted span gets, so consumers don't care how the ancestry appeared.That alone fixes nothing, which is worth being explicit about: both consumers only ask the cache while building their per-span state, and the affected spans already have theirs. So each re-checks state built from an empty answer — the writer on re-entry, the wall profiler in
#getProfilingContext. The writer's re-check is guarded so the announcement published from inside the lookup can't append the endpoint twice.Cost on the hot path: for a resolved span, unchanged. For a span with no web ancestry, two property reads per activation plus the cache's generation compare; an actual re-walk only happens when a promotion has invalidated something.
Jira: PROF-15353