Error: decorateRequest is REMOVED

[nutchalum]

According to https://www.npmjs.com/package/express-http-proxy

Upgrade to 1.0, transition guide and breaking changes

decorateRequest has been REMOVED, and will generate an error when called. See proxyReqOptDecorator and proxyReqBodyDecorator.

Error: decorateRequest is REMOVED; use proxyReqOptDecorator and proxyReqBodyDecorator instead.  see README.md
   at resolveOptions (/usr/src/app/node_modules/express-http-proxy/lib/resolveOptions.js:21:11)
   at new Container (/usr/src/app/node_modules/express-http-proxy/lib/scopeContainer.js:27:14)
   at handleProxy (/usr/src/app/node_modules/express-http-proxy/index.js:39:21)
   at Layer.handle [as handle_request] (/usr/src/app/node_modules/express/lib/router/layer.js:95:5)
   at trim_prefix (/usr/src/app/node_modules/express/lib/router/index.js:312:13)
   at /usr/src/app/node_modules/express/lib/router/index.js:280:7
   at param (/usr/src/app/node_modules/express/lib/router/index.js:349:14)
   at param (/usr/src/app/node_modules/express/lib/router/index.js:365:14)
   at Function.process_params (/usr/src/app/node_modules/express/lib/router/index.js:410:3)
   at next (/usr/src/app/node_modules/express/lib/router/index.js:271:10)

[codefromthecrypt]

Is there a way to handle both versions? or are we going to have to lock to one or the other (or double-publish)

…

On Wed, Nov 8, 2017 at 6:12 PM, nutchalum.k ***@***.***> wrote: According to https://www.npmjs.com/package/express-http-proxy Upgrade to 1.0, transition guide and breaking changes decorateRequest has been REMOVED, and will generate an error when called. See proxyReqOptDecorator and proxyReqBodyDecorator. Error: decorateRequest is REMOVED; use proxyReqOptDecorator and proxyReqBodyDecorator instead. see README.md at resolveOptions (/usr/src/app/node_modules/express-http-proxy/lib/resolveOptions.js:21:11) at new Container (/usr/src/app/node_modules/express-http-proxy/lib/scopeContainer.js:27:14) at handleProxy (/usr/src/app/node_modules/express-http-proxy/index.js:39:21) at Layer.handle [as handle_request] (/usr/src/app/node_modules/express/lib/router/layer.js:95:5) at trim_prefix (/usr/src/app/node_modules/express/lib/router/index.js:312:13) at /usr/src/app/node_modules/express/lib/router/index.js:280:7 at param (/usr/src/app/node_modules/express/lib/router/index.js:349:14) at param (/usr/src/app/node_modules/express/lib/router/index.js:365:14) at Function.process_params (/usr/src/app/node_modules/express/lib/router/index.js:410:3) at next (/usr/src/app/node_modules/express/lib/router/index.js:271:10) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#151>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD61yRKbzYQSy_sAvYZmPB0AfyG3n9gks5s0X6JgaJpZM4QWIye> .

[nutchalum]

We can simply release a new version and create a compatibility table in README.

• v0.10 will support only express-http-proxy < 1.0.0
• v0.10+ will support only express-http-proxy >= 1.0.0

and the new one will support only the latest express-http-proxy

[codefromthecrypt]

We can simply release a new version and create a compatibility table in README. - v0.10 will support only express-http-proxy < 1.0.0 - v0.10+ will support only express-http-proxy >= 1.0.0 and the new one will support only the latest express-http-proxy

ack this is an option. Would like to hear from folks.. can you do a quick github search (issues or just searching for use of this lib) and ping users? If we get a few nods, let's go with it. The implication is that we won't maintain <1.0.0 in this model

[eirslett]

I guess it should be possible to detect if the function exists or not - and then decide which API to use.

[jcchavezs]

Ping @epallerols

[epallerols]

I see one solution that does not involve a breaking change
Maybe it is something we are forced to do (my knowledge of this library is limited), but the current implementation always adds a decorateRequest (see:
https://github.com/openzipkin/zipkin-js/blob/master/packages/zipkin-instrumentation-express/src/wrapExpressHttpProxy.js#L98)

The check from express-http-proxy is as simple as:
https://github.com/villadora/express-http-proxy/blob/master/lib/resolveOptions.js#L20

We can make it work for both versions without adding a breaking change by doing an options check before assign it:

if (options.decorateRequest) {
 wrappedOptions.decorateRequest = wrapDecorateRequest(instrumentation, originalDecorateRequest);
}

what do you think @adriancole ? could it work? If so I can help implementing it

[codefromthecrypt]

I'm not an expert, but I suspect tests could suss out if it works or not. I don't think we could run multiple versions in our build (at least I don't know a way), but we could fake it and then one of the folks here who are looking for the change could verify. I'd go for it, but still open to others who are smarter than me at javascript.

[codefromthecrypt]

ack this is something we should fix

[jcchavezs]

Now that we have more solid test base it is probably worth to scratch this @epallerols? We can pair on it next week. What do you think?

[jcchavezs]

This is a duplication of #204 by @jonesg504 and #110 by @grisanu

[DarkBitz]

Is any progress to be expected in the near future? The problem still occurs with "express-http-proxy" v1.6.0 and "zipkin" v0.19.1

[jcchavezs]

Unfortunately not, we are mainly volunteers and we don't have fixed time allocated for every project. I would say if you could come up with a failing test case then it will be way easier for us to jump in. Of course if some of you can take a shot on this (you or @epallerols) would be awesome. If none of them is a possibility we will look at it at some point next week but given that this middleware has 19k downloads per month I think there are many people using this that could also jump in.

[DarkBitz]

That would be the update for v1.16.0. For all who are waiting for a fix. I've avoided function definitions in functions as well as possible, this is terrible to read.

const {Request, Annotation} = require('zipkin');
const url = require('url');

class ExpressHttpProxyInstrumentation {

  constructor({tracer, serviceName = tracer.localEndpoint.serviceName, remoteServiceName}) {
    this.tracer = tracer;
    this.serviceName = serviceName;
    this.remoteServiceName = remoteServiceName;
  }

  decorateAndRecordRequest(serverReq, proxyReq, serverTraceId) {

    return this.tracer.letId(serverTraceId, () => {
      const clientTraceId = this.tracer.createChildId();
      this.tracer.setId(clientTraceId);

      const proxyReqWithZipkinHeaders = Request.addZipkinHeaders(proxyReq, clientTraceId);
      Object.defineProperty(
        serverReq, 
        '_trace_id_proxy',
        {
          configurable: false, 
          get: () => clientTraceId
        }
      );
      this._recordRequest(proxyReqWithZipkinHeaders);

      return proxyReqWithZipkinHeaders;
    });
  }

  _recordRequest(proxyReq) {
    this.tracer.recordServiceName(this.serviceName);
    this.tracer.recordRpc(proxyReq.method.toUpperCase());
    this.tracer.recordBinary('http.path', url.parse(proxyReq.path).pathname);
    this.tracer.recordAnnotation(new Annotation.ClientSend());
    if (this.remoteServiceName) {
      this.tracer.recordAnnotation(new Annotation.ServerAddr({
        serviceName: this.remoteServiceName,
        port: parseInt(proxyReq.port)
      }));
    }
  }

  recordResponse(rsp, clientTraceId) {
    this.tracer.letId(clientTraceId, () => {
      this.tracer.recordBinary('http.status_code', rsp.statusCode.toString());
      this.tracer.recordAnnotation(new Annotation.ClientRecv());
    });
  }
}

const wrapProxyReqOptDecorator = (instrumentation, proxyReqOptDecorator) => {

  return (proxyReqOpts, serverReq) => {
    const serverTraceId = serverReq._trace_id;
    let wrappedProxyReqOpts = proxyReqOpts;

    if (typeof proxyReqOptDecorator === 'function') {
      tracer.letId(serverTraceId, () => {
          wrappedProxyReqOpts = proxyReqOptDecorator(proxyReqOpts, serverReq);
      });
    }

    return instrumentation.decorateAndRecordRequest(serverReq, wrappedProxyReqOpts, serverTraceId);
  };
}

const wrapUserResHeaderDecorator = (instrumentation, userResHeaderDecorator) => {

  return (headers, userReq, userRes, proxyReq, proxyRes) => {
    const serverTraceId = userReq._trace_id;
    let wrappedHeaders = headers;

    if (typeof userResHeaderDecorator === 'function') {
      tracer.letId(serverTraceId, () => {
        wrappedHeaders = userResHeaderDecorator(headers, userReq, userRes, proxyReq, proxyRes)
      }); 
    }
    instrumentation.recordResponse(proxyRes, userReq._trace_id_proxy);

    return wrappedHeaders;
  };
}

const wrapProxy = (proxy, {tracer, serviceName, remoteServiceName}) => {

  return function zipkinProxy(host, options = {}) {

    const instrumentation = new ExpressHttpProxyInstrumentation({tracer, serviceName, remoteServiceName});
    const wrappedOptions = options;

    const {proxyReqOptDecorator} = wrappedOptions;
    wrappedOptions.proxyReqOptDecorator = wrapProxyReqOptDecorator(instrumentation, proxyReqOptDecorator);

    const {userResHeaderDecorator} = wrappedOptions;
    wrappedOptions.userResHeaderDecorator = wrapUserResHeaderDecorator(instrumentation, userResHeaderDecorator);

    return proxy(host, wrappedOptions);
  };
}

module.exports = wrapProxy;