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

URL parameterization not taking into account express routers in v8 #12103

Closed
3 tasks done
Tracked by #12109
richardsimko opened this issue May 17, 2024 · 9 comments · Fixed by #13948
Closed
3 tasks done
Tracked by #12109

URL parameterization not taking into account express routers in v8 #12103

richardsimko opened this issue May 17, 2024 · 9 comments · Fixed by #13948
Assignees
Labels
Package: node Issues related to the Sentry Node SDK Type: Bug

Comments

@richardsimko
Copy link
Contributor

richardsimko commented May 17, 2024

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/node

SDK Version

8.0.0

Framework Version

Node: 20.12.2
Express: 4.19.2

Link to Sentry event

https://fl101.sentry.io/performance/trace/9bd6ae8a599b0919ccd1941defbcb6f5

SDK Setup

Sentry.init({dsn: process.env.SENTRY_DSN})

Steps to Reproduce

Mount a router with the following:

const apiRoutes = express.Router();
const fooApiRoutes = express.Router();

fooApiRoutes.get('/foo',() => {
});

apiRoutes.use(fooApiRoutes);

app.use('/api', apiRoutes);

Expected Result

Calls to GET /api/foo should be parameterized as such.

Trace collected with 7.x: https://fl101.sentry.io/performance/trace/9f106d65ff644716b32603965c85b61c/?node=span-a36c5a350b874100&node=txn-98cdcb0f6770474aaa27709f1adfe49b&statsPeriod=14d&timestamp=1715939877

Actual Result

They are presented as /foo.

Trace collected with 8.x: https://fl101.sentry.io/performance/trace/9bd6ae8a599b0919ccd1941defbcb6f5/?fov=0%2C112.99991607666016&node=txn-3ead7993a2bf44b99dc67e857ae743ec&statsPeriod=14d&timestamp=1715938309

@github-actions github-actions bot added the Package: node Issues related to the Sentry Node SDK label May 17, 2024
@mydea
Copy link
Member

mydea commented May 17, 2024

Hey, thanks for reporting this.
It seems there are still some rough spots in the new express instrumentation. We'll work on updating this to get back to full feature parity there!

cc @onurtemizkan what is the status for this, do you remember?

@onurtemizkan
Copy link
Collaborator

Thanks for reporting this @richardsimko,

Are you using multiple or nested router instances, and if so, what's the node version you're using?

The last discussion was about some gaps in node versions lower than 16. We can try to fix this upstream on opentelemetry repository.

Could you provide a minimal reproduction if possible, so we can be sure that use is properly covered in the patch?

@mydea
Copy link
Member

mydea commented May 17, 2024

Ah yeah, and related question: What node version do you use?

@richardsimko
Copy link
Contributor Author

richardsimko commented May 17, 2024

Oh I thought I put that in the issue, sorry! I'm using Node 20.12.2 and Express 4.19.2. I updated the issue with this as well.

We are indeed using two levels of routers now that I check more closely, I've updated the example to reflect our real usage.

@AbhiPrasad
Copy link
Member

Yeah we validated this with https:/getsentry/chartcuterie/ - an express service we run in Sentry itself. We'll need to make some upstream changes to OpenTelemetry to fix this!

@AbhiPrasad AbhiPrasad added this to the v8 Instrumentation Bugs milestone May 27, 2024
@AbhiPrasad AbhiPrasad changed the title URL parameterization not taking into account routers in v8 URL parameterization not taking into express account routers in v8 May 29, 2024
@choxnox
Copy link

choxnox commented Jun 27, 2024

Is there any ETA on this?

@richardsimko richardsimko changed the title URL parameterization not taking into express account routers in v8 URL parameterization not taking into account express routers in v8 Jun 27, 2024
@AbhiPrasad
Copy link
Member

AbhiPrasad commented Jun 27, 2024

This is tracked upstream with open-telemetry/opentelemetry-js-contrib#1993 - we need to fix it with OpenTelemetry!

no ETA atm, but we have someone on our side taking a look this week.

@AbhiPrasad
Copy link
Member

PR: open-telemetry/opentelemetry-js-contrib#2319

Copy link
Contributor

A PR closing this issue has just been released 🚀

This issue was referenced by PR #13948, which was included in the 8.35.0-beta.0 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: node Issues related to the Sentry Node SDK Type: Bug
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants