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 - incorrect route set to span, if middlewares are being used #2680

Open
RigoTamas opened this issue Jan 27, 2025 · 4 comments · May be fixed by #2682
Open
Labels
bug Something isn't working has:reproducer This bug/feature has a minimal reproduction provided pkg:instrumentation-express priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect

Comments

@RigoTamas
Copy link

What version of OpenTelemetry are you using?

{
    "@opentelemetry/api": "^1.9.0",
    "@opentelemetry/auto-instrumentations-node": "^0.55.3",
    "@opentelemetry/exporter-metrics-otlp-proto": "^0.57.1",
    "@opentelemetry/exporter-trace-otlp-proto": "^0.57.1",
    "@opentelemetry/instrumentation-express": "^0.47.0",
    "@opentelemetry/sdk-metrics": "^1.30.1",
    "@opentelemetry/sdk-node": "^0.57.1",
    "@opentelemetry/sdk-trace-node": "^1.30.1",
    "express": "4.21.1"
  }

What version of Node are you using?

20.15.0

What did you do?

This repository reproduces the bug:
https://github.com/RigoTamas/opentelemetry-express-instrumentation-span-name-bug

// index.ts

import express, { NextFunction, Request, Response, Router } from "express";

const app = express();

const userMiddleware = (req: Request, res: Response, next: NextFunction) => {
  console.log("user middleware running");
  next();
};

const userMiddleware2 = (req: Request, res: Response, next: NextFunction) => {
  console.log("user middleware 2 running");
  next();
};

const userRouter = Router({ mergeParams: true });

userRouter.use(userMiddleware);

userRouter.use(userMiddleware2);

userRouter.get("/", (req: Request, res: Response, next: NextFunction) => {
  res.send({ name: "John" });
});

const v1Router = Router({ mergeParams: true });

v1Router.use("/user", userRouter);

app.use("/v1", v1Router);

app.listen(8000, () => {
  console.log("api listening on 8000");
});
// instrumentation.ts
import { getNodeAutoInstrumentations } from "@opentelemetry/auto-instrumentations-node";
import { OTLPMetricExporter } from "@opentelemetry/exporter-metrics-otlp-http";
import { OTLPTraceExporter } from "@opentelemetry/exporter-trace-otlp-proto";
import { Resource } from "@opentelemetry/resources";
import { PeriodicExportingMetricReader } from "@opentelemetry/sdk-metrics";
import { NodeSDK } from "@opentelemetry/sdk-node";
import { ATTR_SERVICE_NAME } from "@opentelemetry/semantic-conventions";

const sdk = new NodeSDK({
  traceExporter: new OTLPTraceExporter(),
  metricReader: new PeriodicExportingMetricReader({
    exporter: new OTLPMetricExporter(),
  }),
  resource: new Resource({
    [ATTR_SERVICE_NAME]: "test service",
  }),
  instrumentations: [getNodeAutoInstrumentations()],
});
sdk.start();

What did you expect to see?

I expected to see the http.route tag on the root span, and the name of the root span to be set to GET /v1/user

Image

What did you see instead?

Instead I have seen, that the http.route tag on the root span, and also the name of the root span gets set to GET /

Image

Additional Context

This issue arises because I am calling middlewares on a child router.

@RigoTamas RigoTamas added the bug Something isn't working label Jan 27, 2025
@dyladan dyladan added pkg:instrumentation-express has:reproducer This bug/feature has a minimal reproduction provided labels Jan 29, 2025
@dyladan
Copy link
Member

dyladan commented Jan 29, 2025

@JamieDanielson can you take a look at the associated PR here? You're the listed code owner :)

@dyladan
Copy link
Member

dyladan commented Jan 29, 2025

Is this a duplicate of #2678?

@dyladan dyladan added the priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect label Jan 29, 2025
@RigoTamas
Copy link
Author

I don't think that this is a dupicate of #2678. My issue is related to child routers calling middlewares that don't have any associated path (eg.: childRouter.use(someMiddleware)), while that issue does not include such middlewares. Also, I was not able to reproduce #2678, hence why I created this ticket with a standalone PR. The PR clearly demonstrates my issue with a unit test.

@RigoTamas
Copy link
Author

Also, my issue is not solved with the PR proposed as a solution for #2678, so I strongly believe they are separate issues.

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

Successfully merging a pull request may close this issue.

2 participants