Redesign JS API around the ThreadContext class#17
Merged
ivoanjo merged 11 commits intoJul 16, 2026
Conversation
`CtxWrap` leaks the C++ ObjectWrap implementation detail — as far as the JS API is concerned, the object IS the thread context. Rename: - The JS-visible class name (SetClassName) and the addon export property name: "CtxWrap" -> "ThreadContext". - The prototype's append method: "append" -> "appendAttributes" for parity with the top-level helper's phrasing. Error messages updated accordingly. The native C++ class itself keeps the `CtxWrap` identifier — that's the conventional ObjectWrap-style naming on the C++ side and isn't user-visible. Public JS API surface (runWithContext / enterWithContext / appendAttributes / ...) is unchanged in this commit; just internal identifiers move.
szegedi
marked this pull request as draft
June 24, 2026 13:07
Pure stylistic cleanup ahead of the API redesign so the redesign diff stays focused on semantics: - add 'use strict' pragma - single-quote string literals - drop redundant '= undefined' on let declarations - inline single-statement if-return blocks - collapse blank lines inside asyncContextFrameError - replace 'x == null' with explicit '=== null || === undefined'
szegedi
force-pushed
the
otel-thread-ctx-redesign
branch
from
June 24, 2026 16:37
3e5dfbb to
103a34a
Compare
szegedi
force-pushed
the
otel-thread-ctx-redesign
branch
from
June 25, 2026 12:21
2618881 to
f5fdc1f
Compare
Reshape the top-level surface so consumers can cache one record per
logical scope and re-install it without allocation churn:
- ThreadContext is now a JS-constructable class:
`new threadContext.ThreadContext(traceId, spanId, attrs?)`. Instance
methods are `appendAttributes(attrs)` and `isTruncated()`, plus the
debug-only `debugBytes()`.
- New top-level operations take a context handle (or undefined):
`getContext()`, `setContext(context)`,
`runWithContext(context, fn)`. JS reference identity replaces any
byte-level comparison for "is this context already active".
- Drop the opts-form top-level helpers (`enterWithContext(opts)`,
`appendAttributes(attrs)`, `clearContext()`, `isContextTruncated()`)
in favor of:
setContext(new ThreadContext(...))
context.appendAttributes(...)
setContext(undefined)
getContext()?.isTruncated() ?? false
- NamedContext is now a thin factory: `buildContext(opts)` resolves
`namedAttributes` to a positional array and returns a
ThreadContext; `runWithContext` / `enterWithContext` /
`clearContext` are kept as one-liner sugar that compose with the
top-level functions.
Tests updated to match the new shape; the few tests that exercised
removed guards (TypeError on undefined opts, "no active thread
context" thrown by the removed top-level appendAttributes, etc.) are
rewritten or dropped — those error paths don't exist anymore.
…utes The named-attribute API (buildContext / enterWithContext / runWithContext sugar) added little value once ThreadContexts are constructed explicitly with positional attributes — the positional API is sufficient for our use cases. What remains useful is the process-context attribute snapshot that pairs the on-the-wire uint8 key indexes with the application-side key list, schema version, and V8 layout constants. Expose that as a top-level getProcessContextAttributes(keys) function.
szegedi
force-pushed
the
otel-thread-ctx-redesign
branch
from
June 25, 2026 12:38
f5fdc1f to
de42d31
Compare
szegedi
marked this pull request as ready for review
June 30, 2026 09:32
Four independently-flagged concerns, ported from the equivalent fixes on the pprof-nodejs vendored copy (DataDog/pprof-nodejs#366): - Accept optional third `attributes` arg in ThreadContext constructor. The TS ThreadContextCtor.new type declares `attributes` optional, but the C++ side hard-errored on `args.Length() != 3`. Loosen to accept 2 or 3 args; EncodeAttrs already handled undefined/null attrs_val correctly. - Fold `cleanup_registered` into `undefined_addr`. Uses the field's zero / non-zero state as the 'already-registered' flag: it starts at zero, ResetDiscoveryStruct clears it back to zero (so a re-init on the same thread would re-register), and any real V8 undefined singleton address is non-zero. Removes the separate thread_local static. - Coerce attribute values to strings in a pre-pass in EncodeAttrs before writing to the output buffer. Value->ToString may execute user JS (custom toString methods) which could re-enter into the ThreadContext via appendAttributes and interleave with our writes. Separating the coerce phase from the encode phase keeps the encode phase re-entrancy-free. - Make the 'enterWithContext attaches the record to the current async scope' test callback `async` and `await tcRun(...)`. Previously the callback returned a promise via `tcRun(...)` and the promise's inner `.then` assertion could fire an unhandled rejection instead of a test failure.
umanwizard
reviewed
Jul 6, 2026
ivoanjo
approved these changes
Jul 7, 2026
ivoanjo
left a comment
Collaborator
There was a problem hiding this comment.
👍 LGTM
In hindsight this PR is a bit harder to review with a few things mixed in, but I was able to follow commit-by-commit.
I think at this point we can still tweak and fix things as we're working to get it end-to-end so I think it's a good time to make these kinds of improvements.
- Inlined various tcXxx helpers - renamed a handful of tests to not reference removed APIs - dropped "isContextTruncated returns false outside a context" as it no longer applies
Ports the reentrancy fix from pprof-nodejs PR #366's follow-up: the prior split of EncodeAttrs into a coerce phase + encode phase didn't actually fix the reentrancy in CtxWrap::AppendAttributes. The right fix is to add a bool `encoding_` field on CtxWrap and reject reentrant AppendAttributes with an explicit throw.
ivoanjo
approved these changes
Jul 16, 2026
ivoanjo
left a comment
Collaborator
There was a problem hiding this comment.
👍 Gave a pass on the latest changes, looks good
Collaborator
|
I've synced with Attila on this -- gonna merge this PR and we can keep iterating with other PRs the branch as we find more small improvements :) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
As part of reviews, and actually attempting to integrate this code with a tracer library, I arrived at some changes.
CtxWraptoThreadContextin the JS API. The former name alluded to an implementation detail of how native data is exposed to JS instead of what it is, I think this is much clearer.ThreadContextclass. Basically, I separated allocation from activation. I dropped the implicit-storage helpers so integrators can use one record per span and activate that record when the span becomes active without per-enter allocation. This also allows attributes previously appended to be preserved as the identity of the record remains stable. makeNamedContext proved to be mostly dead weight with explicit instances, so I dropped it and just preserved the process context attributes getter as a top-level function. These changes were informed by an actual attempt to integrate this API with a tracer library (to paraphrase, no planned API survives contact with integration.).d.tsfor the new API surface.There are also two minor issues addressed:
nodejs_v1_devfor the time being and alsonodejs_v1namespace from the threadlocal layout attribute keys (threadlocal.wrapped_object_offset,threadlocal.tagged_size.) Since the schema name already determines the attribute set, having extra namespacing is unnecessary and would also raise an issue of what to do with the extra namespace if the schema version or name changes.Finally, there's some code formatting changes in their own commits:
.clang-formatwith what is basically Google code style and run it onjs/addon.cpp, plus dropped two unusedusing v8::…declarations.index.jsstylistically in-line with how we usually format JS code (single quotes, no explicit undefined assignments on declaration,===instead of==,use stricton top)Test plan
cd js && npm teston Linux (AsyncContextFrame branch)