feat(instrumentation-express): Support non-string routes #2008

onurtemizkan · 2024-03-13T13:01:10Z

Which problem is this PR solving?

This came up while migrating @sentry/node to OpenTelemetry. (Related issue: getsentry/sentry-javascript#10168).

This PR adds support for:

Routes that are defined as regular expressions.
Multiple routes provided as an array.

The current behaviour falls back to / as the layerPath when the Express route argument is not a string, which does not cover all uses (Express docs).

@sentry/node already has a test suite to cover these uses, so this PR migrates them to OpenTelemetry for validation.

Short description of the changes

Adds getLayerPath utility to extract multiple route arguments, including regular expressions.
Adds new ESM tests to validate this use. (Migrated from sentry-javascript)

linux-foundation-easycla · 2024-03-13T13:01:14Z

The committers listed above are authorized under a signed CLA.

✅ login: onurtemizkan / name: Onur Temizkan (d9b35d2, 0eb793c, 54717b6, 83558f0, e4abf27, 3aac5e7, b188318)
✅ login: JamieDanielson / name: Jamie Danielson (81ccd08, 63a79c2)
✅ login: pichlermarc / name: Marc Pichler (5f607ce)

AbhiPrasad · 2024-03-13T15:25:49Z

plugins/node/opentelemetry-instrumentation-express/test/fixtures/use-express-regex.mjs

+const port = server.address().port;
+
+await new Promise(resolve => {
+  http.get(`http://localhost:${port}/test/arr/requiredPath/optionalPath/lastParam`, (res) => {


Can we table test more regex scenarios to see if it works? good to help against regression

https://github.com/getsentry/sentry-javascript/blob/78797919819f6427d667e3955c439ff8e99eeab2/dev-packages/node-integration-tests/suites/express/tracing/test.ts#L94-L101

AbhiPrasad · 2024-03-13T15:26:34Z

plugins/node/opentelemetry-instrumentation-express/test/fixtures/use-express-regex.mjs

+  next();
+});
+
+app.get(['/test/arr/:id', /\/test\/arr[0-9]*\/required(path)?(\/optionalPath)?\/(lastParam)?/], (_req, res) => {


can we have a test for some simpler regex like we do here? https://github.com/getsentry/sentry-javascript/blob/78797919819f6427d667e3955c439ff8e99eeab2/dev-packages/node-integration-tests/suites/express/tracing-experimental/server.js#L25-L31

JamieDanielson

Thanks for this! Overall I think this looks good - I am trying to confirm if there is specific guidance on when to approve the workflows. In the meantime I think this will also require an npm run lint:fix. Can you update that on your side and push, along with an explicit return type for the getLayerPath function?

plugins/node/opentelemetry-instrumentation-express/src/utils.ts

trentm · 2024-03-19T23:11:01Z

I am trying to confirm if there is specific guidance on when to approve the workflows.

FWIW, I've tended to "Approve and run" after a sanity check that the PR doesn't add a bitcoin miner, credentials exfil or somethign like that. :)

onurtemizkan · 2024-03-20T14:00:31Z

@JamieDanielson, thanks for reviewing. I updated the PR.

codecov · 2024-03-20T14:30:13Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.36%. Comparing base (dfb2dff) to head (63a79c2).
Report is 102 commits behind head on main.

❗ Current head 63a79c2 differs from pull request most recent head 5f607ce. Consider uploading reports for the commit 5f607ce to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2008      +/-   ##
==========================================
- Coverage   90.97%   90.36%   -0.62%     
==========================================
  Files         146      147       +1     
  Lines        7492     7687     +195     
  Branches     1502     1576      +74     
==========================================
+ Hits         6816     6946     +130     
- Misses        676      741      +65

Files	Coverage Δ
...try-instrumentation-express/src/instrumentation.ts	`100.00% <100.00%> (ø)`
...opentelemetry-instrumentation-express/src/utils.ts	`98.11% <100.00%> (+0.49%)`	⬆️

... and 9 files with indirect coverage changes

dyladan · 2024-03-25T17:07:13Z

I am trying to confirm if there is specific guidance on when to approve the workflows.

FWIW, I've tended to "Approve and run" after a sanity check that the PR doesn't add a bitcoin miner, credentials exfil or somethign like that. :)

That's also my strategy

dyladan

Overall looks ok to me but I have a little bit of concern that getLayerPath is not resilient to strings and regexes that have the same stringified represenation.

plugins/node/opentelemetry-instrumentation-express/src/utils.ts

dyladan · 2024-03-25T18:49:34Z

plugins/node/opentelemetry-instrumentation-express/src/utils.ts

+ * @param args - Arguments of the route
+ * @returns The layer path
+ */
+export const getLayerPath = (args: unknown[]): string | undefined => {


This seems a bit fragile to me in the sense that it would be pretty easy to get the same output from a regexp that it would be to get from a string path with a trailing slash. Is there any guardrail against this situation, is this an acceptable risk, or is this not actually an issue if it happens? I'm not an expert in this instrumentation and I'm not 100% sure what the layer path is used for.

IMHO it would be an acceptable risk. I'm not sure how to work around this without breaking the path pattern.

Is this still an open question? I'm not sure I'm following the specific concern here, can you provide an example of what we are considering guarding against?

My understanding is, that as this PR converts regexp path definitions to a string representation, it may collide with an exact string path which can make both end up under the same transaction name.

Something like:

/\/test\/regex/ and

/test/regex/ can end up as

/test/regex/

This seems possible, but l don't think it is a common pattern when people define routes.

Just a thought since I am also following this PR: There is always the possibility for collisions when coercing anything into a string. In my opinion this is worth it. If we want to reduce the likelihood of collisions, we could transform regular expressions like const re = /\/test\/regex\/ with const coerced = `RegEx(${re.toString()})`;. 🤔

Hmm... I think I'm in agreement here that this is an acceptable risk as-is, as we are otherwise guarding against edge cases that are unlikely to occur, and if it does occur it is a fairly minimal impact. Especially compared to the current state, this gets us to a much better place. If we find ourselves getting issues related to this we can reconsider, but it might even just end up being a documentation thing at that point.

Co-authored-by: Jamie Danielson <[email protected]>

AbhiPrasad · 2024-04-10T20:29:43Z

@dyladan @JamieDanielson could you take another look at this when you get the time? Thanks!

JamieDanielson

Thank you for the details and adding all the tests. I'm curious if anyone has final objections we haven't addressed but this looks good to me.

onurtemizkan requested a review from a team March 13, 2024 13:01

github-actions bot added the pkg:instrumentation-express label Mar 13, 2024

github-actions bot assigned JamieDanielson and pkanal Mar 13, 2024

github-actions bot requested review from JamieDanielson and pkanal March 13, 2024 13:01

AbhiPrasad reviewed Mar 13, 2024

View reviewed changes

onurtemizkan changed the title ~~feat(instrumentation-express): Support multiple routes with RegExp.~~ feat(instrumentation-express): Support RegExp routes Mar 15, 2024

onurtemizkan changed the title ~~feat(instrumentation-express): Support RegExp routes~~ feat(instrumentation-express): Support non-string routes Mar 15, 2024

onurtemizkan force-pushed the feat-multiple-routes-regex branch from 3522c4a to c0b8441 Compare March 15, 2024 16:34

JamieDanielson reviewed Mar 19, 2024

View reviewed changes

plugins/node/opentelemetry-instrumentation-express/src/utils.ts Outdated Show resolved Hide resolved

JamieDanielson mentioned this pull request Mar 19, 2024

express instrumentation update breaking span names #1893

Closed

onurtemizkan force-pushed the feat-multiple-routes-regex branch from 465180f to 0c1f5a0 Compare March 20, 2024 14:00

onurtemizkan force-pushed the feat-multiple-routes-regex branch from 8a92001 to 6f53b7f Compare March 20, 2024 16:05

dyladan reviewed Mar 25, 2024

View reviewed changes

onurtemizkan and others added 6 commits March 31, 2024 15:04

feat(instrumentation-express): Support multiple routes with RegExp.

b188318

Cover more uses.

d9b35d2

Update plugins/node/opentelemetry-instrumentation-express/src/utils.ts

3aac5e7

Co-authored-by: Jamie Danielson <[email protected]>

Fix linter errors.

54717b6

Add getLayerPath unit tests.

0eb793c

Address review suggestions.

83558f0

onurtemizkan force-pushed the feat-multiple-routes-regex branch from fd16275 to 83558f0 Compare April 1, 2024 15:11

onurtemizkan and others added 2 commits April 16, 2024 14:18

Fix linter errors

e4abf27

Merge branch 'main' into feat-multiple-routes-regex

81ccd08

JamieDanielson approved these changes Apr 22, 2024

View reviewed changes

JamieDanielson and others added 2 commits April 22, 2024 13:35

Merge branch 'main' into feat-multiple-routes-regex

63a79c2

Merge branch 'main' into feat-multiple-routes-regex

5f607ce

pichlermarc merged commit 525bbba into open-telemetry:main May 2, 2024
15 checks passed

dyladan mentioned this pull request May 2, 2024

chore: release main #2172

Merged

nicholaschoi168 mentioned this pull request May 19, 2024

[Snyk] Upgrade @opentelemetry/auto-instrumentations-web from 0.32.3 to 0.39.0 nicholaschoi168/opentelemetry.io#8

Open

snyk-io bot mentioned this pull request May 22, 2024

[Snyk] Upgrade @opentelemetry/auto-instrumentations-web from 0.38.0 to 0.39.0 Hawthorne001/opentelemetry.io#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(instrumentation-express): Support non-string routes #2008

feat(instrumentation-express): Support non-string routes #2008

onurtemizkan commented Mar 13, 2024 •

edited

Loading

linux-foundation-easycla bot commented Mar 13, 2024 •

edited

Loading

AbhiPrasad Mar 13, 2024

AbhiPrasad Mar 13, 2024

JamieDanielson left a comment

trentm commented Mar 19, 2024

onurtemizkan commented Mar 20, 2024

codecov bot commented Mar 20, 2024 •

edited

Loading

dyladan commented Mar 25, 2024

dyladan left a comment

dyladan Mar 25, 2024

onurtemizkan Apr 1, 2024

JamieDanielson Apr 17, 2024

onurtemizkan Apr 18, 2024

lforst Apr 18, 2024

JamieDanielson Apr 22, 2024

AbhiPrasad commented Apr 10, 2024

JamieDanielson left a comment

feat(instrumentation-express): Support non-string routes #2008

feat(instrumentation-express): Support non-string routes #2008

Conversation

onurtemizkan commented Mar 13, 2024 • edited Loading

Which problem is this PR solving?

Short description of the changes

linux-foundation-easycla bot commented Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamieDanielson left a comment

Choose a reason for hiding this comment

trentm commented Mar 19, 2024

onurtemizkan commented Mar 20, 2024

codecov bot commented Mar 20, 2024 • edited Loading

Codecov Report

dyladan commented Mar 25, 2024

dyladan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbhiPrasad commented Apr 10, 2024

JamieDanielson left a comment

Choose a reason for hiding this comment

onurtemizkan commented Mar 13, 2024 •

edited

Loading

linux-foundation-easycla bot commented Mar 13, 2024 •

edited

Loading

codecov bot commented Mar 20, 2024 •

edited

Loading