Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

Fix mongodb instrumentation not ending span correctly #358

Merged
merged 1 commit into from
Mar 4, 2019
Merged

Fix mongodb instrumentation not ending span correctly #358

merged 1 commit into from
Mar 4, 2019

Conversation

vmarchaud
Copy link
Contributor

From my tests all the mongodb queries had the wrong duration so i inspected the code and found out that the span were never ended and automatically truncated when the root span is ended.

So i rewrote a little bit the patchs so they could correctly be ended and added the verification in the tests

cc @draffensperger @mayurkale22

@codecov-io
Copy link

codecov-io commented Feb 24, 2019

Codecov Report

Merging #358 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #358      +/-   ##
=========================================
- Coverage   95.23%   95.2%   -0.04%     
=========================================
  Files         129     129              
  Lines        8592    8598       +6     
  Branches      634     638       +4     
=========================================
+ Hits         8183    8186       +3     
- Misses        409     412       +3
Impacted Files Coverage Δ
src/stackdriver-monitoring.ts 81.05% <0%> (-2.11%) ⬇️
src/mongodb.ts 91.78% <0%> (-0.98%) ⬇️
test/test-mongodb.ts 93.79% <0%> (+0.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fd1a7b...3aff62b. Read the comment docs.

@vmarchaud
Copy link
Contributor Author

Test failures are related to Stackdriver so i believe we can ignore them for this PR

@mayurkale22
Copy link
Member

Test failures are related to Stackdriver so i believe we can ignore them for this PR

I am working on fixing the build.

@mayurkale22
Copy link
Member

Can you please rebase your branch with the master, hopefully it will fix build.

@vmarchaud
Copy link
Contributor Author

I believe the PR can be merged now ?

@vmarchaud
Copy link
Contributor Author

Ping @mayurkale22

@mayurkale22
Copy link
Member

@vmarchaud Thanks for the contributions, PR looks good. If no other comments from @draffensperger I will merge this PR later today.

Copy link
Contributor

@draffensperger draffensperger left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fixes and comments.

@mayurkale22 mayurkale22 merged commit 58eff0f into census-instrumentation:master Mar 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants