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

Request & Response Logger #265

Closed
EwenQuim opened this issue Dec 12, 2024 · 19 comments
Closed

Request & Response Logger #265

EwenQuim opened this issue Dec 12, 2024 · 19 comments
Assignees
Labels
feature-request New feature or request good first issue Good for newcomers

Comments

@EwenQuim
Copy link
Member

Is your feature request related to a problem? Please describe.

A middleware, enabled by default but desactivable via a server option, that logs:

  • when a request enters into the server, showing method, path, params (but not body) and the request id
  • when a response goes out of the server, showing the response code and answer time, and the corresponding request id

You can create a new default_middlewares.go and the option to activate/desactivate it should be in the server.go file

@EwenQuim EwenQuim added good first issue Good for newcomers feature-request New feature or request labels Dec 12, 2024
@dylanhitt
Copy link
Collaborator

couple things to add and question.

Please allow for disablement of one logger or other. In an environment that uses a saas product for their monitoring infra logging both the request and the response can cause a lot of costs! Also I assume the default should be with slog?

@dylanhitt
Copy link
Collaborator

One more though on request id here. What are the thoughts checking if the header of X-Request-ID is on the if there use it if not generate a random uuid?

@EwenQuim
Copy link
Member Author

Yes, yes and yes! Good ideas :)

Also, if the request can be DEBUG and the response INFO, it would be nice !

@dylanhitt
Copy link
Collaborator

Yeah I agree with you after playing around a bit tonight. The request log should just be debug. Trying to make everything configurable isn't worth the squeeze. It ends up just being easier to create your own logger middleware.

@dylanhitt
Copy link
Collaborator

instead of default_middlewares.go I think it should go into middlewares/logger as well.

@EwenQuim
Copy link
Member Author

EwenQuim commented Dec 21, 2024

But it would mean in another module and not included by default, right?

@dylanhitt
Copy link
Collaborator

Hmmm, the more I think about this the more I’m realizing I’m thinking about ding too much.

yeah, let’s just to the default that you described. With a simple on/off. I am imagining middlewares/logger could exist eventually, but that would be a separate effort outside of fuego core. I think this default logger should aim for being just enough.

@EwenQuim
Copy link
Member Author

Yeah just a basic logger will be enough. For advanced use cases, users won't be afraid to create their own.

@jwtly10
Copy link
Contributor

jwtly10 commented Dec 23, 2024

Hey @dylanhitt @EwenQuim

Am I OK to work on this issue?

One question about configurability - do we want to selectively enable/disable either the resp or req logs? Or just a global flag that turns off this middleware completely, so anyone is free to impl their own?

@EwenQuim
Copy link
Member Author

I would say just a global on/off. But if you find a way to do both without too much downsides, you can do it.

I'm open to suggestions, do as you wish! :)

@dylanhitt
Copy link
Collaborator

@jwtly10 I played with the idea of doing it, but couldn't really find a pattern I liked for it. Basically what Ewen said if you can figure out way to do it cleanly please too. Here's some draft PRs of what my approach was.

If not I liked Ewen's idea of setting the request log to Debug.

@jwtly10
Copy link
Contributor

jwtly10 commented Dec 23, 2024

Had a first stab at it, pretty much just building on what you did @dylanhitt
#303

Added some tests to show the API, I've tried to keep it simple and inline with existing options

@aidenwong812
Copy link

Can I try solving this issue?

@mimisavage
Copy link

I'd like to take this issue.

@EwenQuim
Copy link
Member Author

@jwtly10 already did some great work on this issue and it's about to be merged. But #266 can be a nice first contribution!

@Emmy123222
Copy link

Let me try this one!

@Nityam573
Copy link

I’m interested in this one.

@VyuduInc
Copy link

Mind if I try this one?

@dylanhitt
Copy link
Collaborator

closed with #313

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

8 participants