-
Notifications
You must be signed in to change notification settings - Fork 494
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): fix instrumentation of ESM-imported koa #1736
Conversation
Hi, thanks for your contribution and sorry for the late review. # #1735 introduced a simple way to test if everything is working as intended with ESM instrumentations. Would you mind adding this to this PR as well? |
Hi @pichlermarc, I failed to create a proper test case based on the #1735 PR. Is there anyone can help? |
I can help add a test case. |
… spans are created; add asserts for the expected spans
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1736 +/- ##
==========================================
- Coverage 91.49% 91.47% -0.03%
==========================================
Files 144 144
Lines 7388 7394 +6
Branches 1474 1478 +4
==========================================
+ Hits 6760 6764 +4
- Misses 628 630 +2
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this!
Thanks for the reviews. I turned on the TAV tests here and I see that they fail, so I may need to dig into that. It might be a separate issue, though. Update: It was a race in the newly added test. Fixed in commit 06fc6ba. |
The ESM test case runs use-koa.mjs and expects some spans. It relies on the ordering of those spans (based on start time). In some runs two spans were starting in the same millisecond and the sort order was ill-defined, which caused tests to fail.
Thanks @chentsulin! |
Which problem is this PR solving?
Short description of the changes