autotelic / fastify-opentelemetry

A Fastify plugin that utilizes the OpenTelemetry API to provide request tracing.
MIT License
76 stars 12 forks source link

wrapRoutes breaks FastifyInstance <> this binding #28

Closed mjpowersjr closed 3 years ago

mjpowersjr commented 3 years ago

Fastify's default behavior is to bind the this variable to the Fastify Server instance. (see Route Options).

When using this plugin's wrapRoutes option, this is set to undefined when the route handler is invoked. Ideally, the original behavior could be maintained, while using wrapRoutes.

jkirkpatrick24 commented 3 years ago

Hi @mjpowersjr !

Thanks for opening the issue, is there a minimum reproduction you could provide to help us fix this? Or better yet, would you like to contribute a PR?

mjpowersjr commented 3 years ago

Thanks for the quick response. I did take a look at the code, but TBH I'm a bit out of my depth on the opentelemetry side. This is a simple test that could be dropped along side your current tests.

  test('should preserve this binding in handler using wrapRoutes', async ({ is, same, teardown }) => {
    class TestController {
        async handlerRequest(request, reply) {
            if (! this) {
                throw new Error('this is undefined');
            }
            return Object.keys(this)
        }
    } 

    // Note: fastify does not support binding 'this' with arrow functions based router handlers!
    const controller = new TestController();
    const fastify = setupTest({ 
        serviceName: 'test',
        wrapRoutes: true,
     }, controller.handlerRequest);

    const reply = await fastify.inject(injectArgs);
    is(reply.statusCode, 200);

    teardown(() => {
      fastify.close()
    })
  });
HW13 commented 3 years ago

Thanks for the test snippet @mjpowersjr! I was able to track down the bug and have a fix in PR 👍

HW13 commented 3 years ago

Fix is live in v0.11.2 🚀

mjpowersjr commented 3 years ago

Thanks you!