This repository was archived by the owner on Jan 21, 2026. It is now read-only.
fix: force http and https clients to be patched#1084
Merged
JustinBeckwith merged 6 commits intogoogleapis:masterfrom Aug 5, 2019
Merged
fix: force http and https clients to be patched#1084JustinBeckwith merged 6 commits intogoogleapis:masterfrom
JustinBeckwith merged 6 commits intogoogleapis:masterfrom
Conversation
googlebot
added
the
cla: yes
ofrobots
reviewed
Jul 31, 2019
Contributor
ofrobots
left a comment
ofrobots
left a comment
There was a problem hiding this comment.
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?
ofrobots
reviewed
Jul 31, 2019
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
ofrobots
approved these changes
Aug 5, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Requests from
node-fetchcurrently 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 ofhttporhttps. Therefore, forhttp, if it's the only module to be included that would requirehttp, thenhttpwill never get patched, and HTTP requests will never get traced. The same is independently true ofhttps. (Common HTTP frameworks will typically requirehttp, 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
httpandhttpswhen the Trace Agent starts.Fixes #1081