Skip to content

feat: add request layer lifecycle TracingChannels - #196

Merged
UlisesGascon merged 27 commits into
pillarjs:masterfrom
logaretm:tracing-channel
Oct 9, 2026
Merged

UlisesGascon merged 27 commits into
pillarjs:masterfrom
logaretm:tracing-channel

Conversation

@logaretm

@logaretm logaretm commented Apr 15, 2026 •

Copy link
Copy Markdown
Member

This PR adds a single tracing channel that emits structured lifecycle events (start, end, asyncStart, asyncEnd, error) for every middleware, route handler, and error handler execution.

This builds on #96 but uses tracing channels instead which enables context propagation out of the box.

Context shape

The context object is pretty minimal, providing both req and res enables APMs to figure out how to react to that specific payload.

{ req, res, layer, pattern, error, errorHandler }
  • layer is the Layer instance. Subscribers use layer.name and req.route (set when inside a matched route) to classify and name spans however they see fit.
  • pattern is the path the layer was registered with (e.g. /api/:version). layer.path only holds the part of the URL matched for the current request, and the registered path is otherwise discarded once the matchers are compiled, so without it subscribers can't build a parameterized, low-cardinality route (e.g. http.route) for mounted routers.
  • errorHandler is true for error-handling middleware (4-arg signature), whose context also carries the error it received.
  • error is set when the layer fails, whether via next(err), a throw, or a rejected promise. The error event is published once, on the layer where the error originates, not again as it bubbles through outer layers. The 'route' and 'router' routing signals are not errors.

Usage Example:

This is an example I've set up locally using a Sentry setup with this PR patched in:

const Sentry = require('@sentry/node')
// otel-tracing-channel is a simple tracing channel wrapper that patches in the OTEL context propagation
// atm otel doesn't support tracing channels, so this workaround is required for OTEL-based APMs
// the user wouldn't need this and typically they can set their own thing with ALS.
const { tracingChannel } = require('otel-tracing-channel')

Sentry.init({
  // ...
})

// Subscribe to the express.router.request TracingChannel and create Sentry spans
const channel = tracingChannel('express.router.request', (ctx) => {
  const { layer, req } = ctx
  const isErrorHandler = Boolean(ctx.errorHandler)
  const isRouteHandler = Boolean(req.route)

  const spanName = isRouteHandler
    ? `${req.method} ${req.route.path}`
    : isErrorHandler
      ? `error_handler - ${layer.name}`
      : `middleware - ${layer.name}`

  return Sentry.startSpanManual(
    {
      name: spanName,
      op: isRouteHandler ? 'http.handler' : isErrorHandler ? 'middleware.error' : 'middleware',
      attributes: {
        'express.name': layer.name,
        'http.request.method': req.method,
        'url.path': req.url,
        'url.full': req.originalUrl,
        ...(req.route && { 'http.route': req.route.path }),
        ...(req.query && Object.keys(req.query).length > 0 && {
          'url.query': JSON.stringify(req.query)
        }),
        ...(req.params && Object.keys(req.params).length > 0 && {
          'express.params': JSON.stringify(req.params)
        })
      }
    },
    (span) => span
  )
})

channel.subscribe({
  asyncEnd (ctx) {
    if (ctx.req.route) {
      ctx.span?.setAttribute('http.response.status_code', ctx.res.statusCode)
    }
    ctx.span?.end()
  },
  error (ctx) {
    ctx.span?.setStatus({ code: 2, message: ctx.error?.message })
    ctx.span?.end()
  }
})

// Express App
const express = require('express')
const app = express()

// Middleware
app.use(express.json())

app.use(function requestLogger (req, res, next) {
  console.log(`${req.method} ${req.url}`)
  next()
})

app.use(function addRequestId (req, res, next) {
  req.requestId = Math.random().toString(36).slice(2, 10)
  res.setHeader('x-request-id', req.requestId)
  next()
})

// Routes
app.get('/users/:id', function validateId (req, res, next) {
  if (isNaN(req.params.id)) {
    return res.status(400).json({ error: 'id must be a number' })
  }
  next()
}, function loadUser (req, res, next) {
  req.user = { id: req.params.id, name: 'Test User' }
  next()
}, function getUser (req, res) {
  res.json({ ...req.user, requestId: req.requestId })
})

This produces a waterfall view looking like this:

CleanShot 2026-04-15 at 08 36 22@2x

Note: The three spans that look the same at the bottom actually represent the three handlers executing for that route, it is up to the subscriber to decide which one is the route handler and how to name the other ones since it has access to layer.

Is tracing channels experimental?

We had this come up a few times during the initiative of pushing the adoption of dc across the ecosystem, most recently this was brought up and responded to by @Qard here:

Automattic/mongoose#16105 (comment)

Yes, it has mainly been left as experimental due to a lack of adoption, but also as a signal that we may make improvements as we learn from uses of it. That learning has largely already been absorbed and applied via recent PRs at this point. There are several improvements incoming, but none of those change the existing code, which will be left as-is, they add some additional systems to handle some more specific cases such as async iterators.

The TracingChannel API will be left as-is, likely forever at this point as it is very important to us to retain compatibility essentially indefinitely, because APM vendors have many customers using old Node.js versions even well beyond the EOL of Node.js itself [...]

Related work

Performance

Benchmarked in-process on Node 24 (no HTTP), with 12 traced layers per request: 10 middlewares, a router mounted at /api/:version, and a /users/:id route. The traced subscriber binds an AsyncLocalStorage store on start and listens to every event, as an APM would. Each number is the median of 5 separate runs.

Variant Sync handlers (ns/req) Async handlers (ns/req)
master (merge-base) 1730 1983
PR, no subscribers 1785 2002
PR, traced 4464 5025
PR, traced without ALS store 2313 2804

With no subscribers the PR is at parity with master (the differences are within run-to-run noise). When traced, ~75–80% of the overhead comes from runStores entering the AsyncLocalStorage context, which is the cost of async context propagation in Node itself, and the rest from publishing.

Benchmark script
// node bench.js <router dir> <traced 0|1> <sync|async>
const dc = require('node:diagnostics_channel')
const { AsyncLocalStorage } = require('node:async_hooks')
const Router = require(require('node:path').resolve(process.argv[2]))
const [traced, async] = [process.argv[3] === '1', process.argv[4] === 'async']

if (traced) {
  // what an APM typically does: bind an ALS store on start, listen to every event
  const ch = dc.tracingChannel('express.router.request')
  ch.start.bindStore(new AsyncLocalStorage(), ctx => ctx)
  ch.subscribe({ start () {}, end () {}, asyncStart () {}, asyncEnd () {}, error () {} })
}

// 10 middlewares -> mounted router at /api/:version -> /users/:id route (12 traced layers)
const router = new Router()
for (let i = 0; i < 10; i++) router.use(async ? async (req, res, next) => next() : (req, res, next) => next())
const api = new Router()
api.get('/users/:id', async ? async (req, res) => res.end() : (req, res) => res.end())
router.use('/api/:version', api)

const once = () => new Promise((resolve, reject) => {
  router.handle({ method: 'GET', url: '/api/v1/users/42', headers: {} }, { end: resolve }, err => reject(err || new Error('unhandled')))
})

;(async () => {
  const N = 50000
  for (let i = 0; i < N; i++) await once() // warmup
  const rounds = []
  for (let r = 0; r < 5; r++) {
    const t = process.hrtime.bigint()
    for (let i = 0; i < N; i++) await once()
    rounds.push(Number(process.hrtime.bigint() - t) / N)
  }
  console.log('ns/req (median of 5):', rounds.sort((a, b) => a - b)[2].toFixed(0))
})()

Each number in the table is the median of 5 separate runs per variant. The "no ALS store" rows are the same script with the bindStore line removed.

Caveats

  • The channel name is express.router.request. In the original PR it was request, but channel names need a relationship with the module name, otherwise we risk duplication in the ecosystem. It's tied to the express project rather than the pillarjs org name, since the package may move organizations (see the review thread).
  • Depending on the exact version of Node 18 certain behaviors are expected in this PR:
    • TracingChannel#hasSubscribers is undefined before Node 20.13/22, so the router checks each sub-channel's hasSubscribers instead. With no subscribers the traced path is skipped entirely, with no context allocation.
    • Node versions without tracingChannel support will just not offer tracing of any kind. The fallback here is NOOP.
  • It is up to APMs to represent the onion shape with nested handlers or flatten certain spans which is relatively easy if APMs have their own ALS context propagation strategy.
  • Tests running a node version that do not have tracingChannel will be skipped via describe.skip.

Add a single `express:request` TracingChannel that emits structured
lifecycle events (start, end, asyncStart, asyncEnd, error) for every
middleware, route handler, and error handler execution.

Context shape: { req, res, layer } where layer is the Layer instance,
giving subscribers access to layer.name, layer.handle.length (for error
handler detection), and layer.route. Consumers decide how to classify
and name spans based on these properties.

Route dispatch wrapper layers (internal glue that calls route.dispatch)
are excluded from tracing to avoid duplicate events. The actual user
handlers inside the route are traced individually.

Zero overhead when no subscribers are registered. The hasSubscribers
check gates all context allocation and tracePromise wrapping.

Refs: pillarjs#96
Refs: expressjs/express#6353
Extract handlePromise and unify the traced/untraced paths into a
single try/catch and .then handler.
Copilot AI review requested due to automatic review settings April 15, 2026 12:45
logaretm added a commit to getsentry/js-tracing-channels-proposals that referenced this pull request Apr 15, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds request-layer lifecycle instrumentation using Node.js diagnostics_channel TracingChannel, emitting structured events for middleware, route handlers, and error handlers to support APM-style tracing with context propagation.

Changes:

  • Add express:request TracingChannel integration in Layer request/error handling.
  • Introduce shared invocation helper to wrap handler execution and promise handling with tracing hooks.
  • Add a new test suite validating emitted events, context shape, and basic ordering/behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
lib/layer.js Adds TracingChannel wiring and refactors handler invocation through a tracing-aware helper.
test/tracing.js New mocha tests covering tracing context, handler types, and lifecycle event expectations.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/layer.js Outdated
Comment thread lib/layer.js Outdated

@Qard Qard left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Generally LGTM. I think we might want to validate if handled errors are still captured on the span prior to the handler though.

Comment thread test/tracing.js Outdated
})

describe('error handler tracing', function () {
it('should trace error handlers (fn.length === 4)', function (done) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should this one capture the error even though it is considered handled overall?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not entirely sure, APMs will usually have a global error handler or whatever to catch them so having them traced might be an overkill and may duplicate error emissions for those who don't dedup them.

WDYT?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

But does it having a handler here result in the error not being present on the span for the layer that lead to the error handler? Whatever the behaviour is, I think it would be useful to validate it more explicitly in this particular test. We should have a clear model of what errors happen where.

@logaretm logaretm Apr 20, 2026 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks! I added more tests to pin down various behaviors. Here is how errors flow right now:

No error handler (no app.use):

  • Route throws sync -> error channel publishes for the route layer. Router responds 500.
  • Route promise rejects -> error channel publishes for the route layer. Router responds 500.

With error handler:

  • Route throws sync -> error channel publishes for the route, handler runs, nothing on the error channel for the handler.
  • Route rejects async -> same, error channel publishes for the route, handler clean.

With error handler that also throws/rejects:

  • Route throws sync, handler throws -> error channel publishes twice, once per layer. Two legitimately failed executions.
  • Route rejects async, handler throws -> same.

For an APM: both throw cases produce two error events on the channel — one with ctx.layer pointing at the route, one at the error handler. Each event targets its own span, so the APM just marks both spans as failed.

In the case for next(err) then the child error handler span will be marked, not the parent one. Which is fine I suppose since it technically recovered as you've said.

Tests in 1ad1b9c.

@Qard Qard left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hmm...this handling seems a bit incorrect. If an error is unhandled it should definitely be reported. And even if it is handled we should still probably be reporting it so we can capture cases like a too permissive error handler silencing legitimate failures.

I think probably what we should be doing is always capturing the error and just adding some handled flag to the context to mark when it has been handled somehow. That way we can still report it with the detail that the application thinks it has been handled while still allowing human judgement to look at the span and see if that assessment actually makes sense.

Comment thread test/tracing.js Outdated
})
})

it('should not emit error on the originating layer when next(err) is unhandled', function (done) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This seems problematic. Unhandled errors should definitely always be observable.

@logaretm logaretm Apr 20, 2026 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Okay agreed, this could've become a real gap and it's always easier to dedup rather than miss events.

I pushed a fix to that in 90ed7e2 and adjusted the tests to pin that behavior.

@bjohansebas
bjohansebas requested review from efekrskl and krzysdz April 21, 2026 03:07

@Qard Qard left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think it's in a reasonable state now. Might be good to also get a review from @rochdev or @Flarna for some more perspectives from other APM vendors. The specific error semantics I think might be a bit up to interpretation how they should be handled, but I think this shape should generally provide the context that is needed for the different ways we think about route errors. 🤔

Comment thread lib/layer.js Outdated
* @private
*/

const requestChannel = dc.tracingChannel && dc.tracingChannel('express:request')

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why is express:request channel used here?
I think it would be better to use a dedicated channel for router to allow consumers to decide on what to receive and how to handle it.
I think router component can be also used without express.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point, I don't think it has to be express prefixed but it needs a unique prefix for sure which is why I wanted some opinions on what it should be. My concern is router:request is too generic and can easily conflict with other names in the ecosystem.

I had this in the PR description:

The channel name right now is express:request, in the original PR it was request but I think channel names need to have a relationship with the module name otherwise we risk duplication in the ecosystem.

WDYT? Maybe pillarjs:router:request?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There seems to be a guideline.
So I guess the string to use would be pillarjs.router.request as tracing: prefix and event name postfix is added by dc.tracingChannel().

@logaretm logaretm Apr 21, 2026 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Doesn't seem like there is a consistent standard, docs do suggest that but others just went with lib:something:action formats.

Anyways I have no strong opinions here and I think as long as the name is unique enough, it is arbitrary. I renamed it to your suggestion 👍

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it should just be router.request and not pillarjs.router.request. I say this because pillarjs is more like the organization name and that could change, and we might move this package to the expressjs organization (not saying we’ll do it now, but it could happen, so we shouldn’t be tied to the GitHub organization name).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okay, I understand the concern. In that case, instead of using pillarjs, it’s better to use express, since it’s officially under the expressjs project.

How does Fastify handle it?

@logaretm logaretm Apr 27, 2026 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

So should we go for express.request or express.router.request? To me its arbitrary but perhaps you have a strong opinion?

Fastify uses fastify.request.handler as seen here.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think express.request is a bit too generic. There might be other parts in express describing the request lifecycle where the router component is not included.
e.g. express.request.router or express.router.request - tending towards express.request.router like fastify and hierarchic ordering.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Went with express.router.request since it describes the action better "Express's router is handling a request".

But happy to change to whatever that is acceptable to get this through.

Follows the Node.js TracingChannel naming guideline (dot-separated,
module-scoped) and avoids the express-specific prefix since router
is usable outside express.
@Flarna

Flarna commented Apr 23, 2026

Copy link
Copy Markdown

The actual data published by the tracing channel is the Layer instance, req and res objects.

If I understand the router module correct layer is an internal object which gets exposed here. Is there any guarantee that the relevant fields on that object stay intact? In fact they become part of the public interface here.

Also I don't find in the docs here that req.route is a public property. Is it safe for consumers to rely on that as it is seen part of public API and following Semver regarding changes?

In general I would love some section in the docs describing the diag channel and it's emitted data.

@logaretm

Copy link
Copy Markdown
Member Author

Is it safe for consumers to rely on that as it is seen part of public API and following Semver regarding changes?

I can't answer that as it usually up to maintainers to decide this but I would say it does imply a public API contract and should be subject to smever.

As for the req.route, I'm not sure the AI created a snippet with it and it worked 🤷‍♂️

The layer object is strictly private today so this woulda actually expose it publicly.

In general I would love some section in the docs describing the diag channel and it's emitted data.

I think we have 3 options here:

  • Define a curated list of properties and expose those instead as public API.
  • Layer objects become a public API.
  • Continue to emit layer, but document a stable partial of it.

I think that would be up to the maintainers to decide, but all options work for us.

@Qard

Qard commented Apr 26, 2026

Copy link
Copy Markdown

I generally suggest that event contents be considered public API surface, but I would not say that is mandatory as it is very common that diagnostics_channel events would be observing what would otherwise be internal implementation details. I would ask though that any exposure of internals be explicitly called out as internal to inform consumers that they should consume those events with defensive accesses to handle cases where shapes may change.

I don't fully think calling it public API surface is quite the right characterization of the intent.

@bjohansebas

Copy link
Copy Markdown
Member

Also I don't find in the docs here that req.route is a public property. Is it safe for consumers to rely on that as it is seen part of public API and following Semver regarding changes?

Yes, it’s part of the public API (see

router/index.js

Line 281 in cb2a5fc

req.route = route
), so it should follow semver. That means anyone can depend on it. There might not be documentation (which I’ll add to my TODO once I’m back on the project, in case no one else has done it or if it doesn’t exist yet, since I don’t have time to verify it right now).

@bjohansebas bjohansebas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please also add documentation about this in the README.

@logaretm

Copy link
Copy Markdown
Member Author

@bjohansebas I added a section on diagnostic channels and their payload, and marked layer as internal impl.

The flag marks that the layer is an error-handling middleware, which
'handled' read as 'the error was resolved' (misleading for a rethrowing
handler). Rename it to errorHandler in the context, README, and tests.
@bjohansebas

Copy link
Copy Markdown
Member

I'm testing on my local machine, which isn't the most stable environment for benchmarks, but I couldn't find much else to optimize. I also think Fastify and others pay the same cost when using tracePromise, so it seems more like a Node.js overhead than something we can really improve on our side

@logaretm

logaretm commented Jul 15, 2026 •

Copy link
Copy Markdown
Member Author

So I did a test and you are right it is 1.9x faster for me, but only on the sync handlers case. Async handlers are more or less the same.

We could do what we did with GraphQL and strap the whole 5 events with runStores and manual publishes. Let me see if that gives us a win in sync case and passes the context propagation tests, it should.

EDIT: Just did, for sync handlers it will effectively follow traceSync behavior, for non async handlers it will follow the same semantics as tracePromise. This regains 1.9x speed for those sync handlers on my machine.

tracePromise coerces every call into a resolved promise, so it emits an
async phase (and pays for it) even for synchronous layers. Publish start
via start.runStores and the remaining events manually, roughly halving
per-layer tracing overhead for synchronous middleware.

Observable change: a synchronous layer now emits only start/end, not
asyncStart/asyncEnd. Consumers should treat end as the terminal event for
a sync layer and asyncEnd for an async one, rather than assuming an async
phase always fires. Routing, error dedup, signal filtering, error
normalization, and async context propagation are unchanged.
@bjohansebas

Copy link
Copy Markdown
Member

SGTM. @Qard @Flarna , since you have more context on this as well, do you think this is a good approach?

@logaretm

logaretm commented Aug 6, 2026

Copy link
Copy Markdown
Member Author

@UlisesGascon any chance we can get this merged? many involved folks already signed off.

logaretm added a commit to getsentry/js-tracing-channels-proposals that referenced this pull request Aug 21, 2026
db0 v0.4.0 and nuxt v4.5.0 both ship their TracingChannels now, which
closes out the last two open non-Sentry PRs. openai-node#1819 was closed
as not planned: no first-party diagnostics_channel while OpenAI designs
the instrumentation contract.

Smaller moves: pillarjs/router#196 is approved, knex is queued for v4.0
once 3.3.0 ships, anthropic closed a stream-only community PR in favour of
one design on #1036, hono's maintainer prefers a 3rd-party middleware over
core, hapi has a stale 2023 community PR, and mysql2/redis/ioredis have
been released for months but still read as merged-only.

Regenerated the coverage block (+2.5 pts adopted today, was +1.6) and
rebuilt the progress summary, which counted openai as not-started and
summed to 45 of 46.
@UlisesGascon

Copy link
Copy Markdown
Member

After discussing this with @logaretm in the collab summit I think we should ship this in the next release. I will have a look in detail into de the implementation by next week and see if we can arrange a a release. I think there are few more things that we want to include including (staged publication on npm).

@wesleytodd wesleytodd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am not going to block by doing "request changes" but I think this still has a bit of work to do. So once y'all are satisfied that the comments in this PR are resolved please feel free to proceed.

Comment thread lib/layer.js Outdated
Comment thread lib/layer.js Outdated
Comment thread lib/layer.js
Comment thread lib/layer.js Outdated
this.name = fn.name || '<anonymous>'
this.params = undefined
this.path = undefined
this.pattern = path

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Did we get a bad rebase here?

@logaretm logaretm Oct 1, 2026 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

So I added this in baba06d, was one missing gap that I added just now rather than make another later PR for it.

The idea is we need a way to parameterize the URL for low cardinality attributes, if you think its bad idea to put it on the layer, I can place on module-level map but could be a bit leaky.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

no, not that it is a bad idea. I just think it should be landed as a separate PR. I actually thought we already had a PR for this. So might be worthing looking around for that.

@logaretm logaretm Oct 4, 2026 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Got it, I see that #174 does add a similar rawPath property. I dropped it from this PR. I can create a quick follow up to include it in the channel payload once #174 gets merged.

@UlisesGascon UlisesGascon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! As discussed on Slack the other things will be covered in a separate PR 👍

@logaretm

logaretm commented Oct 6, 2026 •

Copy link
Copy Markdown
Member Author

Node 19 has a bug where the last subscriber unsubs it drops the channel object so new subs get nothing. I will push a workaround (adding a noop sub that survives cleanup between tests).

Node 19 drops a channel once its last subscriber unsubscribes, so the
router's cached channel stopped receiving events after the first test.
@UlisesGascon
UlisesGascon merged commit 8759eec into pillarjs:master Oct 9, 2026
10 checks passed
logaretm added a commit to getsentry/js-tracing-channels-proposals that referenced this pull request Oct 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support diagnostic channels

7 participants