DataDog / dd-trace-js

JavaScript APM Tracer
https://docs.datadoghq.com/tracing/
Other
650 stars 306 forks source link

request hooks only expose IncomingMessage and ServerResponse #1270

Open bizob2828 opened 3 years ago

bizob2828 commented 3 years ago

Describe the bug I was trying to decorate a trace in Koa with the parsed query params, body, url params and found it will not work by implementing the request hook. This will only work in express and any of the express derivatives that decorate the IncomingMessage with additional context like query params, body, url params.

const tracer = require('dd-trace').init()

tracer.use('koa', {
  hooks: {
    request: (span, ctx) => {
      span.setTag('body', ctx.request.body)
      span.setTag('queryParams', ctx.request.query)
      span.setTag('urlParams', ctx.params)
    }
  }
})

After digging more it appears this is because the koa plugin(hapi and fastify as well) only instrument the IncomingMessage and ServerResponse. I'd open a PR but not sure the direction here as this is a first class hook and the signature would break if we passed in the framework specific context.

Here's a snippet on how I got this working in Koa:

module.exports = function init(Koa, tracer) {
      const origHandleRequest = Koa.prototype.handleRequest;
      Koa.prototype.handleRequest = function (...args) {
          const [ctx] = args;
          const origEnd = ctx.res.end;
          ctx.res.end = function (...args) {
              const scope = tracer.scope();
              const span = scope.active();
              if (span && ctx?.request?.body) {
                  span.setTag('http.body', ctx.request.body);
              }

              if (span && ctx?.request?.query) {
                  span.setTag('http.query', ctx.request.query);
              }

              if (span && ctx?.params) {
                  span.setTag('http.url_params', ctx.params);
              }

              return origEnd.apply(this, args);
          };

          return origHandleRequest.apply(this, args);
      };
  };

Environment

rochdev commented 3 years ago

I'd open a PR but not sure the direction here as this is a first class hook and the signature would break if we passed in the framework specific context.

Since Koa also uses the request and response objects from Node, it would be possible to add the context at the end, so the signature wouldn't break, for example request: (span, req, res, ctx) => {}. The complicated part will be that the Koa plugin uses a generic helper that doesn't know about Koa's context, so the helper will need to be improved to accept additional arguments for hooks.

bizob2828 commented 3 years ago

I'd open a PR but not sure the direction here as this is a first class hook and the signature would break if we passed in the framework specific context.

Since Koa also uses the request and response objects from Node, it would be possible to add the context at the end, so the signature wouldn't break, for example request: (span, req, res, ctx) => {}. The complicated part will be that the Koa plugin uses a generic helper that doesn't know about Koa's context, so the helper will need to be improved to accept additional arguments for hooks.

@rochdev what if make the 4th arg a generic frameworkCtx? This would place nice with fastify and Hapi as well. I can update all places that uses the generic web.instrument

rochdev commented 3 years ago

There might be frameworks with more than one context objects or none at all, so I'm not sure about limiting it to one. However, that would just be an implementation detail that can later be changed if needed, so I'd say go for it if it's the easiest way to unblock accessing the Koa context from the hook.

bizob2828 commented 3 years ago

There might be frameworks with more than one context objects or none at all, so I'm not sure about limiting it to one. However, that would just be an implementation detail that can later be changed if needed, so I'd say go for it if it's the easiest way to unblock accessing the Koa context from the hook.

Got it. I worked around the issue with the code snippet above. But I can open a PR to add a 4th arg to web.instrument and update Koa to pass in ctx

rochdev commented 3 years ago

A PR would definitely be welcome! We should be able to include this in 0.32.0.

pushchris commented 3 years ago

@bizob2828 did you ever get around to creating a PR adding that 4th option? Would love to have access to the full Koa context here since the current implementation is limiting

rochdev commented 3 years ago

@pushchris In the meantime, one workaround you can use is to add the context on the request object so that it's available in the hook. For example:

app.use(ctx => {
  ctx.req.ctx = ctx;
});

Then it will be available to the hook:

tracer.use('koa', {
  hooks: {
    request: (span, req) => {
      // context is available on req.ctx
      // for example, to get the Koa request object you would use req.ctx.request
    }
  }
})
gaganza commented 2 years ago

Hello DD team, we are also experiencing issues with tracing requests with a Koa ran service. Unfortunately we do not have the luxury of being able to add the context to the request as a workaround as stated on the previous comment since we are using Strapi (which uses Koa under-the-hood) so I cannot easily mangle the request to add the context.

Here is an example the plain tracer config I am using to try to get custom tags to show up but they are not.



const tracer = require('dd-trace');
tracer.init({ runtimeMetrics: true }); // initialized in a different file to avoid hoisting.

tracer.use('koa', {
  hooks: {
    request: (span, req, res) => {
      span?.setTag('testing', 'testing-tag');
    },
  },
});



 

We are simply requiring this file at the top of server.js (Strapi’s entry point). This is on Strapi v3.6.8 and dd-trace v1.5.1.
 

 Screen Shot 2021-12-20 at 4 28 03 PM Screen Shot 2021-12-20 at 4 32 36 PM

rochdev commented 2 years ago

@gaganza Is the problem that you can't access the Koa context, or that you can't add any tag at all in the hook?

For the former, you should be able to set the context on the request object with a Strapi middleware.

For the latter, if the tag is not added it would mean the hook is not running at all. This is always caused by dd-trace being initialized after the import of the instrumented module, which can be fixed by loading dd-trace earlier. In some frameworks it can be a bit difficult to do this in code but you can always initialize the tracer directly at the CLI level with node -r dd-trace/init server.js or NODE_OPTIONS='-r dd-trace/init'.