fix: correctly disable Express instrumentation #972
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
nwalters512
commented
Apr 12, 2022
Comment on lines
+291
to
+297
| const customMiddleware: express.RequestHandler = (req, res, next) => { | ||
| for (let i = 0; i < 1000; i++) { | ||
| continue; | ||
| } | ||
| return next(); | ||
| }; | ||
| app.use(customMiddleware); |
There was a problem hiding this comment.
I believe we could further simplify this test by removing this.
There was a problem hiding this comment.
I think it's here to ensure that the instrumentation is also disabled on the app level, not only router level.
Treating Express as a black box and keeping at least one of those middleware makes sense imho.
Codecov Report
@@ Coverage Diff @@
## main #972 +/- ##
==========================================
+ Coverage 95.91% 96.50% +0.58%
==========================================
Files 13 19 +6
Lines 856 1086 +230
Branches 178 230 +52
==========================================
+ Hits 821 1048 +227
- Misses 35 38 +3
|
rauno56
reviewed
Apr 13, 2022
rauno56
approved these changes
Apr 13, 2022
rauno56
changed the title
dyladan
approved these changes
Apr 14, 2022
vmarchaud
approved these changes
Apr 15, 2022
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which problem is this PR solving?
Fixes #970.
Short description of the changes
I changed from calling
this._unwrapwithmoduleExports.Router.prototypeto calling it withmodule.Router. This is consistent with how we callthis._wrap.While investigating this, I discovered that the existing test for disabling was broken. It never actually called
instrumentation.disable(), and in fact, callinginstrumentation.disable()did not produce any change in the test's behavior. I changed the test to use theserverWithMiddleware, which AFAICT is necessary for the instrumentation to produce any spans. The test now fails as expected if you remove theinstrumentation.disable()that I added.While comparing tests, I realized that the
should create a child span for middlewarestest has some unused, unnecessary code. I opted to remove that too, though I'd be happy to shift that to another PR if preferred.Checklist
npm run test-all-versionsfor the edited package(s) on the latest commit if applicable.