Skip to content

Redesign JS API around the ThreadContext class#17

Merged
ivoanjo merged 11 commits into
polarsignals:otel-thread-ctx-wipfrom
szegedi:otel-thread-ctx-redesign
Jul 16, 2026
Merged

Redesign JS API around the ThreadContext class#17
ivoanjo merged 11 commits into
polarsignals:otel-thread-ctx-wipfrom
szegedi:otel-thread-ctx-redesign

Conversation

@szegedi

@szegedi szegedi commented Jun 24, 2026

Copy link
Copy Markdown

Summary

As part of reviews, and actually attempting to integrate this code with a tracer library, I arrived at some changes.

  • I renamed CtxWrap to ThreadContext in 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.
  • Most significantly, I also reshaped the JS API around explicitly-allocated instances of ThreadContext class. 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.)
  • Updated README + .d.ts for the new API surface.

There are also two minor issues addressed:

  • Following some discussions, we renamed the schema-version string to nodejs_v1_dev for the time being and also
  • dropped the nodejs_v1 namespace 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:

  • I adopted a .clang-format with what is basically Google code style and run it on js/addon.cpp, plus dropped two unused using v8::… declarations.
  • I brought index.js stylistically in-line with how we usually format JS code (single quotes, no explicit undefined assignments on declaration, === instead of ==, use strict on top)

Test plan

  • cd js && npm test on Linux (AsyncContextFrame branch)
  • Same suite on macOS / Windows (no-op platform branches)
  • Verify the dd-trace-js and pprof-nodejs vendored copies still build against this API

`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
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 added 4 commits June 25, 2026 14:37
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
szegedi force-pushed the otel-thread-ctx-redesign branch from f5fdc1f to de42d31 Compare June 25, 2026 12:38
@szegedi
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.
Comment thread js/addon.cpp Outdated

@ivoanjo ivoanjo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 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.

Comment thread js/test/test.js Outdated
szegedi added 4 commits July 10, 2026 13:38
- 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 ivoanjo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Gave a pass on the latest changes, looks good

@ivoanjo

ivoanjo commented Jul 16, 2026

Copy link
Copy Markdown
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 :)

@ivoanjo
ivoanjo merged commit ed5b891 into polarsignals:otel-thread-ctx-wip Jul 16, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants