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

feat(instrumentation-koa): identify middleware functions inside route handlers #2037

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

satazor
Copy link
Contributor

@satazor satazor commented Mar 24, 2024

Which problem is this PR solving?

Considering the following route handler:

const someMiddleware = () => async function some(ctx, next) => {
  await next();
});

router.get('/post/:id', someMiddleware(), (ctx) => {
  ctx.body = `Post id: ${ctx.params.id}`;
});

...will generate two spans with the same name: router - /post/:id, even though the first handler is actually a named middleware (some).

Ideally, it would generate two different spans:

  • middleware - some
  • router - /post/:id

Short description of the changes

The changes are quite simple: when iterating these handlers, we assume they are a middleware if it's not the last handler on the stack and if it has a name.

@satazor satazor requested a review from a team as a code owner March 24, 2024 15:25
@satazor satazor force-pushed the enhancement/koa-middleware-inside-router branch from 023ac93 to eee63df Compare March 24, 2024 15:26
@satazor
Copy link
Contributor Author

satazor commented Mar 24, 2024

cc @dyladan

Not sure who can look into this since it seems there's no maintainer for this package.

@satazor satazor force-pushed the enhancement/koa-middleware-inside-router branch from eee63df to a4cba8d Compare March 24, 2024 15:31
Copy link

codecov bot commented Mar 24, 2024

Codecov Report

Merging #2037 (a4cba8d) into main (dfb2dff) will increase coverage by 0.00%.
Report is 15 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2037   +/-   ##
=======================================
  Coverage   90.97%   90.98%           
=======================================
  Files         146      145    -1     
  Lines        7492     7455   -37     
  Branches     1502     1489   -13     
=======================================
- Hits         6816     6783   -33     
+ Misses        676      672    -4     
Files Coverage Δ
...lemetry-instrumentation-koa/src/instrumentation.ts 95.45% <100.00%> (+0.05%) ⬆️

... and 5 files with indirect coverage changes

@satazor
Copy link
Contributor Author

satazor commented Mar 25, 2024

Shamelessly pinging @pichlermarc and @trentm 😅

@satazor
Copy link
Contributor Author

satazor commented Apr 2, 2024

cc @blumamir

@satazor
Copy link
Contributor Author

satazor commented May 8, 2024

Bump :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants