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

Default API metrics for NestJS with fastify records full url path instead of full route #245

Open
hesham-desouky opened this issue Apr 22, 2022 · 3 comments
Labels
bug Something isn't working good first issue Good for newcomers Hacktoberfest

Comments

@hesham-desouky
Copy link

hesham-desouky commented Apr 22, 2022

NestJS version: 8.0.0
nestjs-otel version: 3.0.1

NestJS-otel initialization code

    OpenTelemetryModule.forRoot({
      metrics: {
        hostMetrics: true, // Includes Host Metrics
        defaultMetrics: true, // Includes Default Metrics
        apiMetrics: {
          enable: true, // Includes api metrics
          timeBuckets: [], // You can change the default time buckets
          ignoreRoutes: ['/favicon.ico'], // You can ignore specific routes (See https://docs.nestjs.com/middleware#excluding-routes for options)
          ignoreUndefinedRoutes: true, //Records metrics for all URLs, even undefined ones
        },
      },
    })

NestJS App Initialization code

  const app = await NestFactory.create<NestFastifyApplication>(
    AppModule,
    new FastifyAdapter({ logger: false }),
  );

The default metric API collects the full URL instead of the route URL

example
/api/user/1 will be reported as /api/user/1 instead of /api/user/:id

The metric endpoint generate thousands of metrics for each id parameters instead of 1 metric only for this specific route

When i debugged the code, i found out that the default metric middle ware can't retrieve the route object from the request object.

@pragmaticivan pragmaticivan added bug Something isn't working good first issue Good for newcomers labels May 29, 2022
@fabiozig
Copy link

any plans for this to be fixed?

@fabiozig
Copy link

fabiozig commented Jan 4, 2023

I did some research on this, and apparently, the issue is that fastify uses middie to support middlewares, and instead of using the express request object, it uses the req.raw value. That's why the req.route value in the code below is undefined when using fastify.
https://github.com/pragmaticivan/nestjs-otel/blob/main/src/middleware/api-metrics.middleware.ts#L99

here is an explanation of the middie/fastify/middleware problem
nestjs/nest#9927 (comment)

It seems like if we use interceptors instead we would be able to get the fastify request and then the route we want...

const fastifyRequest = context.switchToHttp().getRequest()
fastifyRequest.routerPath...

ttshivers added a commit to ttshivers/nestjs-otel that referenced this issue Jun 22, 2023
Add a metrics interceptor so it can get the proper route path
for fastify applications.

See pragmaticivan#245
@ttshivers
Copy link

ttshivers commented Jun 22, 2023

I worked around this by adding route.path to the fastify raw request with an interceptor.

import type { CallHandler, ExecutionContext, NestInterceptor } from '@nestjs/common';
import { Injectable } from '@nestjs/common';
import type { FastifyRequest, RawRequestDefaultExpression } from 'fastify';
import type { Observable } from 'rxjs';

interface RequestRoute {
  route: {
    path: string;
  };
}

/**
 * Add the router path so it is visibile in the middleware as req.route.path which
 * nestjs-otel uses for route metrics.
 * See https://github.com/pragmaticivan/nestjs-otel/issues/245
 */
@Injectable()
export class FastifyRoutePathInterceptor implements NestInterceptor {
  intercept(context: ExecutionContext, next: CallHandler): Observable<unknown> {
    const request = context.switchToHttp().getRequest<FastifyRequest>();

    // eslint-disable-next-line no-type-assertion/no-type-assertion
    const raw = request.raw as RawRequestDefaultExpression & RequestRoute;
    raw.route = {
      path: request.routeOptions.url,
    };

    return next.handle();
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers Hacktoberfest
Projects
None yet
Development

No branches or pull requests

4 participants