Skip to content

otel-thread-ctx: address review feedback from #347#366

Draft
szegedi wants to merge 1 commit into
mainfrom
szegedi/otel-thread-ctx-review-followup
Draft

otel-thread-ctx: address review feedback from #347#366
szegedi wants to merge 1 commit into
mainfrom
szegedi/otel-thread-ctx-review-followup

Conversation

@szegedi

@szegedi szegedi commented Jul 3, 2026

Copy link
Copy Markdown

Follow-up to review comments on #347 that landed after merge. Four independent, non-API-surface fixes.

Changes

  • 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 < 2 || > 3; EncodeAttrs already handled undefined/null correctly. (review comment)

  • Fold cleanup_registered into undefined_addr. Uses the field's zero / non-zero state as the "already-registered" flag: it starts at zero (thread-local zero-init), 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 bool.

  • Coerce attribute values to strings in a pre-pass in EncodeAttrs, before touching the output buffer. Value->ToString may execute user JS (custom toString) which could re-enter into the same 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 void tcRun(...) and the promise's inner .then assertion could fire as an unhandled rejection instead of a proper test failure.

Test plan

  • npm run test:docker — 150/150 passing (unchanged)
  • Would also be good to add an explicit re-entrancy test (attribute value with a side-effecting toString that calls appendAttributes), but the two-phase encode already prevents interleaving; leaving that as a possible follow-up.

Related

These same concerns likely apply to the upstream vendored copy in polarsignals/custom-labels (PR #17). I'll port them there next.

Four independently-flagged concerns:

- 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 `void tcRun(...)` and the promise's inner `.then`
  assertion could fire an unhandled rejection instead of a test failure.
@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown

Overall package size

Self size: 2.4 MB
Deduped: 3.1 MB
No deduping: 3.1 MB

Dependency sizes | name | version | self size | total size | |------|---------|-----------|------------| | pprof-format | 2.2.2 | 500.53 kB | 500.53 kB | | source-map | 0.7.6 | 185.63 kB | 185.63 kB | | node-gyp-build | 4.8.4 | 13.86 kB | 13.86 kB |

🤖 This report was automatically generated by heaviest-objects-in-the-universe

szegedi added a commit to szegedi/custom-labels that referenced this pull request Jul 3, 2026
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.
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.

1 participant