Skip to content

feat: implement catchall middleware#19

Open
NiklasMerz wants to merge 4 commits intoplathanus-tech:masterfrom
NiklasMerz:catchall-middleware
Open

feat: implement catchall middleware#19
NiklasMerz wants to merge 4 commits intoplathanus-tech:masterfrom
NiklasMerz:catchall-middleware

Conversation

@NiklasMerz
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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

@NiklasMerz
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

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

@leandrodesouzadev
Copy link
Copy Markdown
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