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: enable root span route instrumentation without any express layer spans #273

Merged
merged 11 commits into from
Jan 7, 2021

Conversation

shyimo
Copy link
Contributor

@shyimo shyimo commented Dec 4, 2020

Which problem is this PR solving?


In addition to #176 PR, We should able to instrument the root span name when ignoring all express layers (router, middleware and request_handler).
This significantly reduces the amount of spans for a large and complex express based micro-service with large amount of outgoing http requests for a single incoming http request and still able to set the route on the root span.

Short description of the changes


using express with only request_handler layer before the change:
Screen Shot 2020-12-04 at 11 25 16

using express without request_handler, middleware and route layer (root span name not modified) before the change:
Screen Shot 2020-12-04 at 11 28 59

using express without request_handler, middleware and route layer after the change:
Screen Shot 2020-12-04 at 11 30 19

@shyimo shyimo requested a review from a team December 4, 2020 09:30
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 4, 2020

CLA Signed

The committers are authorized under a signed CLA.

@codecov
Copy link

codecov bot commented Dec 4, 2020

Codecov Report

Merging #273 (5c2dc21) into master (130985b) will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #273      +/-   ##
==========================================
+ Coverage   95.32%   95.35%   +0.03%     
==========================================
  Files         115      115              
  Lines        5948     5991      +43     
  Branches      586      591       +5     
==========================================
+ Hits         5670     5713      +43     
  Misses        278      278              
Impacted Files Coverage Δ
node/opentelemetry-plugin-express/src/express.ts 97.19% <0.00%> (+0.05%) ⬆️
.../opentelemetry-plugin-express/test/express.test.ts 99.44% <0.00%> (+0.16%) ⬆️

@shyimo shyimo force-pushed the express-plugin-enhancements branch from d4a6da6 to 8fe3977 Compare December 6, 2020 07:04
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very different behavior but it doesn't look like you added any configuration options. Am I missing something?

@shyimo
Copy link
Contributor Author

shyimo commented Dec 9, 2020

Hi @dyladan.
Thats true, i didn't add any new configuration to the plugin.
The change only verify that we will modify the root span name with the current request path (for example: HTTP POST -> POST user/:userid) when not using the request_handler express layer.
currently, the root span name is modified only when request_handler layer is on and this adds many tiny spans that sometimes is not needed

@shyimo shyimo requested a review from dyladan December 10, 2020 07:21
@shyimo shyimo requested a review from obecny December 15, 2020 08:55
@shyimo shyimo requested review from dyladan and obecny January 7, 2021 08:03
@shyimo
Copy link
Contributor Author

shyimo commented Jan 7, 2021

@obecny Hi. i fix your comments on the unit test structure

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thx for those changes

@dyladan dyladan added the enhancement New feature or request label Jan 7, 2021
@dyladan dyladan merged commit 31d7bee into open-telemetry:master Jan 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants