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

feat: implement catchall middleware #19

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

NiklasMerz
Copy link
Contributor

@NiklasMerz NiklasMerz commented Nov 18, 2022

Some open todos:

  • Naming of middleware: Is there a better name than "catchall"?
  • Documentation
  • Logging: Do we need to handle different loggers like the other methods?
  • Tests?

Closes #10

@leandrodesouzadev
Copy link
Contributor

Hey @NiklasMerz, first of all thanks for putting your time into this.
I think this is a great start. The only thing that i think it's missing is, the DRF integration will not be catched on this middleware, right?

@NiklasMerz
Copy link
Contributor Author

Good point. In my testing normal class-based views and a function base view with @api_view(["GET"]) work so far. I will have a look.

@leandrodesouzadev
Copy link
Contributor

Also, this Catch all middleware is only applying the metrics. The Error tracing, you think as a separate middleware?

@NiklasMerz
Copy link
Contributor Author

Hey @NiklasMerz, first of all thanks for putting your time into this. I think this is a great start. The only thing that i think it's missing is, the DRF integration will not be catched on this middleware, right?

I added code to also track DRF requests. Seems to work fine in my project.

Also, this Catch all middleware is only applying the metrics. The Error tracing, you think as a separate middleware?

Having one Middleware to do everything automagically in the background sounds good to me. I could also extend the ErrorTracingMiddleware to make this work. What do you think?

@leandrodesouzadev
Copy link
Contributor

I think that this Inheritance will make it less clear what the middleware is actually doing.
Maybe we can leave the CatchAllMiddleware without any Inheritance, and create the others middlewares instances on __init__, then on process_exception/__call__ just call the both middlewares there.

@NiklasMerz
Copy link
Contributor Author

I am not entirely sure what you mean, but feel free to refactor my branch as you like. I quite like the inheritance for solving this and making it clear is just a matter of documentation. But the code has to be maintainable, so we can make it as you prefer.

@NiklasMerz NiklasMerz marked this pull request as ready for review November 25, 2022 13:09
@NiklasMerz
Copy link
Contributor Author

I added some documentation of the new middleware to README.

@leandrodesouzadev What do you think? Do you want to change the code or should I refactor it?

@NiklasMerz
Copy link
Contributor Author

Little ping @leandrodesouzadev. Is there anything I can do to move this PR forward?

@leandrodesouzadev
Copy link
Contributor

Hey @NiklasMerz sorry for not giving attention to this.

Maybe something like this (haven't tested it):

class CatchAllMiddleware:
    def __init__(self, get_request):
        others = [ApmMetricsMiddleware(get_request), ErrorTraceMiddleware(get_request)]

    def __call__(self, request):
         for other in others:
              other(request)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Track views without the mixin
2 participants