Skip to content
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: force http and https clients to be patched #1084

Merged
merged 6 commits into from
Aug 5, 2019

Conversation

kjin
Copy link
Contributor

@kjin kjin commented Jul 31, 2019

Requests from node-fetch currently might not get traced, because we use it in the Trace Agent (for authentication). Because it's loaded in the Trace Agent before we set up monkeypatching, we don't have a way to hook into its require of http or https. Therefore, for http, if it's the only module to be included that would require http, then http will never get patched, and HTTP requests will never get traced. The same is independently true of https. (Common HTTP frameworks will typically require http, alleviating this problem for HTTP requests. However, in a Cloud Functions environment, this might not apply, as even the HTTP framework is required before the Trace Agent has a chance to set up monkeypatching.)

We address this simply by requiring http and https when the Trace Agent starts.

Fixes #1081

@kjin kjin requested a review from a team July 31, 2019 00:39
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 31, 2019
Copy link
Contributor

@ofrobots ofrobots left a comment

Choose a reason for hiding this comment

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

I see the change, and I see that it fixes an issue with tracing of node-fetch, however, I don't see an explanation for why existing code wasn't working and why the change here fixes it. Can you include background on what the bug was and elaborate on why force-loading http and https fixes it?

@codecov
Copy link

codecov bot commented Aug 2, 2019

Codecov Report

Merging #1084 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1084      +/-   ##
==========================================
+ Coverage   94.93%   94.96%   +0.03%     
==========================================
  Files          96       98       +2     
  Lines        6397     6441      +44     
  Branches      498      499       +1     
==========================================
+ Hits         6073     6117      +44     
  Misses        166      166              
  Partials      158      158
Impacted Files Coverage Δ
test/plugins/test-trace-http.ts 100% <100%> (ø) ⬆️
test/plugins/test-trace-node-fetch.ts 100% <100%> (ø)
test/web-frameworks/express-secure.ts 100% <100%> (ø)
src/tracing.ts 90.32% <100%> (+0.32%) ⬆️

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 c24faa3...f175c01. Read the comment docs.

@JustinBeckwith JustinBeckwith merged commit 3ac0b90 into googleapis:master Aug 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

node-fetch requests do not get traced
4 participants