https.get seems to break the b3 propagation and stackdriver propagation libraries

[loneparadox]

https.get seems to break propagation libraries

What version of OpenCensus are you using?

"@opencensus/exporter-stackdriver": "0.0.17",
"@opencensus/nodejs": "0.0.17",
"@opencensus/propagation-b3": "0.0.17"
"@opencensus/propagation-stackdriver": "0.0.17"

What version of Node are you using?

v10.16.1

What did you do?

The following code should reproduce the error:

const tracing = require('@opencensus/nodejs');
const propagation = require('@opencensus/propagation-b3');
const b3 = new propagation.B3Format();

// const propagation = require('@opencensus/propagation-tracecontext');
// const traceContext = new propagation.TraceContextFormat();
// const propagation = require('@opencensus/propagation-stackdriver');
// const sd = propagation.v1;

tracing.start({propagation: b3});

const http = require('http');

//create a server object:
http.createServer(function (req, res) {
  const https = require('https');
  https.get('https://www.google.com', (resp) => {
    let data = '';

    resp.on('data', (chunk) => {
      data += chunk;
    });

    resp.on('end', () => {
      console.log(data.substr(0,20));

      res.write('Hello World!');
      res.end();
    });

  }).on("error", (err) => {
    console.log("Error: ", err);
    res.write(err.message);
    res.end();
  });

}).listen(3000);

What did you expect to see?

What did you see instead?

The above code will break with Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client

Additional context

Notice if you change the above code to:

const tracing = require('@opencensus/nodejs');
const propagation = require('@opencensus/propagation-b3');
const b3 = new propagation.B3Format();

// const propagation = require('@opencensus/propagation-tracecontext');
// const traceContext = new propagation.TraceContextFormat();
// const propagation = require('@opencensus/propagation-stackdriver');
// const sd = propagation.v1;

tracing.start({propagation: b3});

const http = require('http');

//create a server object:
http.createServer(function (req, res) {
  const https = require('https');
  let new_req = https.request({hostname: 'www.google.com', port:443}, (resp) => {
    let data = '';

    resp.on('data', (chunk) => {
      data += chunk;
    });

    resp.on('end', () => {
      console.log(data.substr(0,20));

      res.write('Hello World!');
      res.end();
    });

  }).on("error", (err) => {
    console.log("Error: ", err);
    res.write(err.message);
    res.end();
  });

  new_req.end();
}).listen(3000);

i.e. using the request with a request.end it does not break.

[mayurkale22]

Thanks for reporting this! If you are feeling motivated, maybe you can attempt to write solution and open a PR.

[loneparadox]

@mayurkale22 I took a quick look and if one puts the same shimmer.wrap on the get as it is the http plugin it seems to work.

The current code in the compiled js for the https plugin looks something like:

        if (semver.satisfies(this.version, '>=8.0.0')) {
            shimmer.wrap(this.moduleExports, 'get', this.getPatchHttpsOutgoingRequest());
        }

When you look at the http plugin it looks like:

        if (semver.satisfies(this.version, '>=8.0.0')) {
            shimmer.wrap(this.moduleExports, 'get', () => {
                // Re-implement http.get. This needs to be done (instead of using
                // getPatchOutgoingRequestFunction to patch it) because we need to
                // set the trace context header before the returned ClientRequest is
                // ended. The Node.js docs state that the only differences between
                // request and get are that (1) get defaults to the HTTP GET method and
                // (2) the returned request object is ended immediately. 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
                // https://github.com/googleapis/cloud-trace-nodejs/blob/master/src/plugins/plugin-http.ts#L198
                return function getTrace(options, callback) {
                    const req = http_1.request(options, callback);
                    req.end();
                    return req;
                };
            });

So in the https plugin if you change the code to something similar it seems to work:

        if (semver.satisfies(this.version, '>=8.0.0')) {
//            shimmer.wrap(this.moduleExports, 'get', this.getPatchHttpsOutgoingRequest());
            shimmer.wrap(this.moduleExports, 'get', () => {
                // Re-implement http.get. This needs to be done (instead of using
                // getPatchOutgoingRequestFunction to patch it) because we need to
                // set the trace context header before the returned ClientRequest is
                // ended. The Node.js docs state that the only differences between
                // request and get are that (1) get defaults to the HTTP GET method and
                // (2) the returned request object is ended immediately. 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
                // https://github.com/googleapis/cloud-trace-nodejs/blob/master/src/plugins/plugin-http.ts#L198
                return function getTrace(options, callback) {
                    const req = https.request(options, callback);
                    req.end();
                    return req;
                };
            });

This got me thinking about the original change... which I then found. From #117 a PR was created in https://github.com/census-instrumentation/opencensus-node/pull/324/files which only seemed to solve the problem for the http plugin; and the https plugin was missed.

This is then confirmed by #324 (comment) but was not fixed in that PR.