refactor: use setHeader to set trace context header #643

kjin · 2018-01-11T02:18:25Z

Fixes #639

The HTTP tracing plugin should not be trying to clone the HTTP request options object if it's not even clear if the request is being traced. This change ~~moves the clone to after this check has been made~~ removes the merge call in favor of using setHeader to mutate headers after http.request has been called.

codecov · 2018-01-11T02:49:22Z

Codecov Report

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

@@            Coverage Diff             @@
##           master     #643      +/-   ##
==========================================
+ Coverage   96.03%   96.11%   +0.07%     
==========================================
  Files          31       31              
  Lines        1615     1621       +6     
  Branches      290      289       -1     
==========================================
+ Hits         1551     1558       +7     
+ Misses         64       63       -1

Impacted Files	Coverage Δ
root/project/build/src/plugins/plugin-http2.js	`95.23% <0%> (ø)`	⬆️
root/project/build/src/plugins/plugin-http.js	`100% <0%> (+1.23%)`	⬆️

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 202d4cb...6f7a591. Read the comment docs.

jinwoo · 2018-01-11T03:08:41Z

This change improves the performance only when the client code is not tracing. The performance degradation still happens if the code is traced, right? I'm not sure if #639 is only for the former, not for the latter. Is there a way to improve for both cases?

I'd imagine most production services would want to enable tracing all the time (at least for some of its traffic), and they wouldn't want the performance to be impacted severely.

kjin · 2018-01-11T04:29:30Z

@jinwoo As long we're claiming a guarantee that the HTTP request options object is not being mutated (this could be up for discussion in the spirit of improving performance), I'm not sure there's a way around cloning the headers object. I'm not sure what the traffic patterns are like in #639 but it might very well be possible that the number of outgoing HTTP requests indeed far exceeds the number of traced requests (we by default limit it to 10 traces/second).

evanlucas · 2018-01-11T14:26:29Z

@kjin would yall actually consider an option for mutating the request object? That seems like it would be the smallest performance hit. #639 is a socket.io server that makes outbound http requests on every socket pong that is received, so yes, the outbound requests greatly exceed the inbound requests for most cases.

I also agree with @jinwoo. It would be ideal to tune for the production if we can :]

jinwoo · 2018-01-11T17:15:25Z

@kjin Do you mean that this new code will degrade the performance only for 10 requests/second at maximum, while the old code will degrade for all the requests? If that is the case, I think this is in a much better state, if not ideal.

kjin · 2018-01-11T18:16:27Z

Looking through the docs I'm reminded that http.setHeader exists. As the implementation of http.request internally copies the headers object already (at least in v4/6/8/9) we might be able to have our cake and eat it too :)

jinwoo

CI is failing.

src/plugins/plugin-http.ts

@@ -65,21 +61,27 @@ function makeRequestTrace(request, api) {
      return request.apply(this, arguments);
    }

-    options = isString(options) ? url.parse(options) : merge({}, options);
-    options.headers = options.headers || {};
+    var uri;


ofrobots · 2018-01-11T18:40:22Z

@evanlucas

@kjin would yall actually consider an option for mutating the request object?

This was a bug. We were cloning the request options for all requests rather than the sampled requests. I expect that this change will fix the performance issue even without the change to avoid mutation.

kjin · 2018-01-11T21:43:55Z

@GoogleCloudPlatform/node-team @evanlucas PTAL

jinwoo

LGTM

src/plugins/plugin-http.ts

+      // The former is already true (at least in supported Node versions up to
+      // v9), so we simply follow the latter.
+      // Ref: https://nodejs.org/dist/latest/docs/api/http.html#http_http_get_options_callback
+      return function makeGetTrace() {


kjin · 2018-01-12T21:55:16Z

CircleCI is backed up... a previous Node 6 build passed (differences are not specific to Node 6) https://circleci.com/gh/GoogleCloudPlatform/cloud-trace-nodejs/1289

Landing.

refactor: clone http options after root span check

3aaf700

kjin requested review from ofrobots, jinwoo and DominicKramer January 11, 2018 02:18

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 11, 2018

refactor: use setHeader to set trace context header

1e9ae9e

kjin changed the title ~~refactor: clone http options after root span check~~ refactor: use setHeader to set trace context header Jan 11, 2018

jinwoo reviewed Jan 11, 2018

View reviewed changes

kjin force-pushed the http-options-lazy-clone branch 3 times, most recently from c0adcc1 to 5b5d696 Compare January 11, 2018 19:52

refactor: account for get

b7dbeef

kjin force-pushed the http-options-lazy-clone branch from 5b5d696 to b7dbeef Compare January 11, 2018 20:58

jinwoo approved these changes Jan 11, 2018

View reviewed changes

evanlucas reviewed Jan 11, 2018

View reviewed changes

refactor: don't call apply for outgoing http requests

3d2d054

evanlucas approved these changes Jan 12, 2018

View reviewed changes

chore: disable http2 for node 9.4+

6f7a591

kjin force-pushed the http-options-lazy-clone branch from 239e530 to 6f7a591 Compare January 12, 2018 19:31

kjin merged commit 182c0cb into googleapis:master Jan 12, 2018

kjin mentioned this pull request Jan 12, 2018

chore: 2.4.1 release proposal #646

Merged

kjin mentioned this pull request Jun 1, 2018

fix: inject context http headers early if expect header exists #766

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use setHeader to set trace context header #643

refactor: use setHeader to set trace context header #643

kjin commented Jan 11, 2018 •

edited

codecov bot commented Jan 11, 2018 •

edited

jinwoo commented Jan 11, 2018

kjin commented Jan 11, 2018

evanlucas commented Jan 11, 2018

jinwoo commented Jan 11, 2018

kjin commented Jan 11, 2018

jinwoo left a comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

ofrobots commented Jan 11, 2018

kjin commented Jan 11, 2018

jinwoo left a comment

This comment was marked as spam.

This comment was marked as spam.

kjin commented Jan 12, 2018

refactor: use setHeader to set trace context header #643

refactor: use setHeader to set trace context header #643

Conversation

kjin commented Jan 11, 2018 • edited

codecov bot commented Jan 11, 2018 • edited

Codecov Report

jinwoo commented Jan 11, 2018

kjin commented Jan 11, 2018

evanlucas commented Jan 11, 2018

jinwoo commented Jan 11, 2018

kjin commented Jan 11, 2018

jinwoo left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

ofrobots commented Jan 11, 2018

kjin commented Jan 11, 2018

jinwoo left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

kjin commented Jan 12, 2018

kjin commented Jan 11, 2018 •

edited

codecov bot commented Jan 11, 2018 •

edited