open-telemetry / opentelemetry-js-contrib

OpenTelemetry instrumentation for JavaScript modules
https://opentelemetry.io
Apache License 2.0
677 stars 496 forks source link

@opentelemetry/instrumentation-express middlewares registered on routers do not record spans unless middlewares applied to router in a chain #1834

Open bratushkadan opened 9 months ago

bratushkadan commented 9 months ago

What version of OpenTelemetry are you using?

v.1.17.0 @opentelemetry/instrumentation-express version is 0.33.1

What version of Node are you using?

v16.18.1

What did you do?

I applied middlewares to express.Router via variadic arguments and array, and instrumentation only seems to record spans for the latter of registered middlewares.

I will provide as much details as I possibly can in the "Additional context" section.

What did you expect to see?

span for MiddlewareA, ...B, ...C, ...D.

What did you see instead?

span for MiddlewareD

Additional context

Case # 1

const apiRouter = express.Router()

const middlewareA0 = async (req, res, next) => {
  await wait(35)
  next()
}

const middlewareA = async (req, res, next) => {
  await wait(50)
  next()
}
const middlewareB = async (req, res, next) => {
  await wait(25)
  next()
}
const middlewareC = async (req, res, next) => {
  await wait(100)
  next()
}
const middlewareD = async (req, res, next) => {
  await wait(70)
  next()
}

/* any of the following ways of registering middlewares yields
same result - span is recorded only for "middlewareD" */
apiRouter.use(middlewareA0, [middlewareA, middlewareB, middlewareC, middlewareD])
// or
apiRouter.use([middlewareA, middlewareB, middlewareC, middlewareD])
// or
apiRouter.use(...[middlewareA, middlewareB, middlewareC, middlewareD])

Result:

Screen Shot 2023-11-29 at 10 49 35

Case # 2

// ...

const apiOrdersRouter = express.Router()

const middlewareE1 = async (req, res, next) => {
  await wait(40)
  next()
}
const middlewareE2 = async (req, res, next) => {
  await wait(80)
  next()
}
const middlewareE3 = async (req, res, next) => {
  await wait(70)
  next()
}

apiOrdersRouter.use(middlewareE1, middlewareE2, middlewareE3)
Screenshot

Potential workaround

// replace
apiRouter.use([middlewareA, middlewareB, middlewareC, middlewareD])
apiOrdersRouter.use(middlewareE1, middlewareE2, middlewareE3)
// with
for (const m of [middlewareE1, middlewareE2, middlewareE3]) {
  apiOrdersRouter.use(m)
}
for (const m of [middlewareA, middlewareB, middlewareC, middlewareD]) {
  apiRouter.use(m)
}

The result is behavior, expected to be with other ways of registering middleware too:

img
dyladan commented 9 months ago

@JamieDanielson @pkanal can you please look into this?

JoshuaCS94 commented 1 month ago

... so?