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

fixes http.get patches for header propagation #117 #324

Merged
merged 2 commits into from
Feb 18, 2019
Merged

fixes http.get patches for header propagation #117 #324

merged 2 commits into from
Feb 18, 2019

Conversation

vmarchaud
Copy link
Contributor

@vmarchaud vmarchaud commented Feb 3, 2019

I got the same issue as #117 while integrating the tracing system into my APM.
I did go over https://github.com/googleapis/cloud-trace-nodejs/ to checkout their implementation and found a details that could explain the problem: https://github.com/googleapis/cloud-trace-nodejs/blob/master/src/plugins/plugin-http.ts#L198

The test are passing for one good reason: they are all nocked, so the actual http code never run. I added a test that make a GET request to google.fr, it might be better to spawn a local server to avoid instability due to network.

@codecov-io
Copy link

Codecov Report

Merging #324 into master will increase coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #324      +/-   ##
==========================================
+ Coverage   94.93%   94.99%   +0.06%     
==========================================
  Files         119      119              
  Lines        8172     8195      +23     
  Branches      732      734       +2     
==========================================
+ Hits         7758     7785      +27     
+ Misses        414      410       -4
Impacted Files Coverage Δ
test/test-http.ts 98.44% <0%> (-0.99%) ⬇️
src/http.ts 91.5% <0%> (+1.7%) ⬆️
src/stackdriver-monitoring.ts 84.21% <0%> (+3.15%) ⬆️

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 c10212f...e5ca654. Read the comment docs.

@vmarchaud
Copy link
Contributor Author

Not related but i believe the problem is the same with https.get, might need another PR

@vmarchaud
Copy link
Contributor Author

@draffensperger @mayurkale22 Sorry for the ping but do the PR need something specific to be reviewed and merged ?
I wanted to contribute to the node implementation since i'm using it for my APM but the time it take to make a change merged is quite long.

@draffensperger
Copy link
Contributor

Hi @vmarchaud, thanks for the contribution and sorry for the slow response! I suspect this is just a procedural issue where both @mayurkale22 missed this PR since it had others tagged on it. Mayur, is it possible to change the default reviewers for new PRs?

This looks good to me, though Mayur will need to do the final review and merge.

@mayurkale22
Copy link
Member

Hey @vmarchaud, sorry for the late response. PR looks good for me. @draffensperger I think it is possible using CODEOWNERS file. https://help.github.com/articles/about-code-owners/

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