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

Error: decorateRequest is REMOVED #151

Open
nutchalum opened this issue Nov 8, 2017 · 13 comments · May be fixed by #462
Open

Error: decorateRequest is REMOVED #151

nutchalum opened this issue Nov 8, 2017 · 13 comments · May be fixed by #462
Assignees

Comments

@nutchalum
Copy link

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
Copy link
Member

codefromthecrypt commented Nov 9, 2017 via email

@nutchalum
Copy link
Author

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
Copy link
Member

codefromthecrypt commented Nov 9, 2017 via email

@eirslett
Copy link
Contributor

eirslett commented Nov 9, 2017

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

@jcchavezs
Copy link
Contributor

Ping @epallerols

@epallerols
Copy link

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
Copy link
Member

codefromthecrypt commented Apr 12, 2018 via email

@codefromthecrypt
Copy link
Member

ack this is something we should fix

@codefromthecrypt codefromthecrypt self-assigned this Jun 28, 2019
@jcchavezs
Copy link
Contributor

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
Copy link
Contributor

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

@DarkBitz
Copy link

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
Copy link
Contributor

jcchavezs commented Nov 22, 2019 via email

@DarkBitz
Copy link

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;

@jcchavezs jcchavezs pinned this issue Nov 27, 2019
@jcchavezs jcchavezs linked a pull request Jan 29, 2020 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants