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

fix(koa): End span and record exception on a middleware exception #281

Merged
merged 3 commits into from
Jan 18, 2021

Conversation

oguzbilgener
Copy link
Contributor

@oguzbilgener oguzbilgener commented Dec 15, 2020

Fixes #279

Not sure why the lint task failed, I did run npm run lint:fix prior to comitting.

ESLint couldn't find the config "./node_modules/gts" to extend from. Please check that the name of the config is correct.

The config "./node_modules/gts" was referenced from the config file in "/home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/detectors/node/opentelemetry-resource-detector-github/.eslintrc.js".

Which problem is this PR solving?

This PR fixes the problem where .end() is never called on the span for a middleware layer when the middleware layer throws an exception / rejects a promise.

The master branch fails on the test case that I added.

Short description of the changes

  • Always call span.end() for the span that's created for a middleware layer. Also record the exception in the span (as an event), then rethrow it so the application can handle it.

I considered using the safeExecuteInTheMiddle utility function, but it doesn't look like it works well with async functions.

@oguzbilgener oguzbilgener requested a review from a team December 15, 2020 04:47
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 15, 2020

CLA Signed

The committers are authorized under a signed CLA.

@codecov
Copy link

codecov bot commented Dec 15, 2020

Codecov Report

Merging #281 (4a0d41f) into master (0fe7949) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #281      +/-   ##
==========================================
+ Coverage   95.39%   95.41%   +0.01%     
==========================================
  Files         115      115              
  Lines        6061     6087      +26     
  Branches      591      592       +1     
==========================================
+ Hits         5782     5808      +26     
  Misses        279      279              
Impacted Files Coverage Δ
.../node/opentelemetry-koa-instrumentation/src/koa.ts 98.11% <0.00%> (+0.07%) ⬆️
...opentelemetry-koa-instrumentation/test/koa.test.ts 99.30% <0.00%> (+0.13%) ⬆️

@vmarchaud vmarchaud merged commit f8d8686 into open-telemetry:master Jan 18, 2021
@dyladan dyladan added the bug Something isn't working label Jan 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Koa] Span never ends when a middleware layer throws an exception
4 participants