Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
bratushkadan opened this issue Nov 29, 2023 · 2 comments
Assignees
Labels
bug Something isn't working pkg:instrumentation-express priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect

Comments

@bratushkadan
Copy link

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

@bratushkadan bratushkadan added the bug Something isn't working label Nov 29, 2023
@dyladan
Copy link
Member

dyladan commented Nov 29, 2023

@JamieDanielson @pkanal can you please look into this?

@dyladan dyladan added the priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect label Nov 29, 2023
@JoshuaCS94
Copy link

... so?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pkg:instrumentation-express priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect
Projects
None yet
Development

No branches or pull requests

6 participants