-
Notifications
You must be signed in to change notification settings - Fork 544
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(express): listen for finish
event on response for async express layer #107
#188
fix(express): listen for finish
event on response for async express layer #107
#188
Conversation
Codecov Report
@@ Coverage Diff @@
## master #188 +/- ##
==========================================
+ Coverage 95.21% 95.26% +0.05%
==========================================
Files 92 87 -5
Lines 4723 4541 -182
Branches 488 476 -12
==========================================
- Hits 4497 4326 -171
+ Misses 226 215 -11 |
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.
lgtm, just have few question to understand the changes better
e97aa05
to
7c8f8fe
Compare
@obecny Sorry for the force push, i didn't saw your review and wanted to make a review before someone would do one :/ |
This PR has been open for a while now and it contains a important bug fix, can we merge it or do we need more review ? |
1a0014c
to
6d23d61
Compare
I was hesitant of implement it like this from the start (for performance reason since we are adding a listener for every async layer) but seeing the half-broken (timing are not accurate) spans reporting, i think it's better like this for end user.