ericjeker / nestjs-sentry-example

Explanations and example project on how to setup Sentry in a Nest.js project
MIT No Attribution
83 stars 16 forks source link

Request Scoped DI Tree slows down the API significantly #4

Closed parsasi closed 11 months ago

parsasi commented 1 year ago

Thank you for creating this. I just wanted to warn that using this will cause pretty much all dependencies to be Request Scoped. I'd recommend not using this approach in production.

The Problem

By making the providers request-scoped, and injecting them to AppModule, API will instantiate new injection models for everything. This can slow down the API significantly.

Using request-scoped providers will have an impact on application performance. While Nest tries to cache as much metadata as possible, it will still have to create an instance of your class on each request. Hence, it will slow down your average response time and overall benchmarking result. Unless a provider must be request-scoped, it is strongly recommended that you use the default singleton scope.

The Solution

Use the context from the intercept method of the Exception Filter to start the Sentry Transaction and set the span.

Extract the request from the SentryInterceptor.

@Injectable()
export class SentryInterceptor implements NestInterceptor {
  constructor(private sentryService: SentryService) {}

  intercept(context: ExecutionContext, next: CallHandler): Observable<any> {
    const request = context.switchToHttp().getRequest();

     const span = this.sentryService.getRequestSpan(request, {
      op: `Route Handler`,
    });

    return next.handle().pipe(
      catchError((error) => {
        // capture the error, you can filter out some errors here
        Sentry.captureException(
          error,
          span.getTraceContext(),
        );

        // throw again the error
        return throwError(() => error);
      }),
      finalize(() => {
        span.finish();
      }),
    );
  }
}

Modify the sentry.service to accept the request as a parameter, and return the scope:

@Injectable()
export class SentryService {
    public getRequestSpan(request: Request, spanContext: SpanContext) {
    const { method, headers, url } = request;

    const transaction = Sentry.startTransaction({
      name: `Route: ${method} ${url}`,
      op: 'transaction',
    });

    Sentry.getCurrentHub().configureScope((scope) => {
      scope.setSpan(transaction);

      scope.setContext('http', {
        method,
        url,
        headers,
      });
    });

    const span = Sentry.getCurrentHub().getScope().getSpan();

    span.startChild(spanContext);

    return span;
  }
}
ericjeker commented 1 year ago

Thanks @parsasi, I'll check on that. I haven't noticed a big impact on performance using the proposed setup but obviously if it's easy to improve it we should definitely use an optimized solution.

ericjeker commented 1 year ago

Hi @parsasi , I did some load testing and the performance started to get consistently worse on pretty high load. I made a PR with your solution above. Thanks for that. Do you want to author the PR?

Btw I published the load testing scripts in the master branch if you wanna try.

parsasi commented 1 year ago

Hi @ericjeker, I'd love to author the PR. I'll have it in a couple of days. Thanks!

[UPDATE] I see you've already made it. I'll wait for future opportunities for contributions :)

ericjeker commented 11 months ago

Merged the PR.