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

Dynamic log level based on http status of request #276

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

Conversation

rbotchek
Copy link

@rbotchek rbotchek commented Feb 8, 2019

Lograge is great, but I need the log level to reflect whether or not the request is processed successfully. This PR adds Lograge.map_log_level (default false). When map_log_level is set to true, the Lograge.log_level is ignored and the log level is set based on the HTTP status as:

  • status < 300 is :info
  • 300 <= status < 400 is :warn
  • 400 <= status < 500 is :error
  • status >= 500 is :fatal

@askaliuk-square
Copy link

+1

@iloveitaly
Copy link
Collaborator

@rbotchek this is a cool idea! Can you rebase on master so CI runs? Would love to get this merged in.

@NeilsC
Copy link

NeilsC commented Mar 30, 2022

I'd love to see this feature added. If @rbotchek is not available to rebase it, I'd be happy to take a swing at it.

@iloveitaly
Copy link
Collaborator

@NeilsC go for it!

@rbotchek
Copy link
Author

rbotchek commented Mar 30, 2022

Hi @iloveitaly and @NeilsC ...

I wasn't ignoring you, but it has been three years without a reply since I created this PR, so I wasn't rushing either. :)

Two comments:

  • I'd be very happy if @NeilsC wants to take a swing at re-basing the PR on the current master. I have projects that are using my personal fork (rbotchek/lograge), but my fork is way behind roidrage/lograge. So, I second the "go for it" to @NeilsC !
  • In the 3 years since creating this PR I've had more thoughts about the overall usefulness. I originally created the PR because at the time I was working on three interrelated RoR-based API servers. For API servers it made (makes?) a lot of sense to map the HTTP status to a log level, because in an API server almost any unusual status is worth knowing about. However, I have more recently experimented with this "map-log-level" feature on traditional UI-focused RoR servers. In a UI-facing server there are actually quite a few redirects (302) and bot- or hacker-triggered 4xx errors that happen in the "normal" course of operation. In these contexts I doubt the usefulness of this map-log-level, because it creates quite a lot of WARN/ERROR noise in the logs that isn't warranted.

Anyway, happy to see that at least a few people are interested in this. And I still think lograge is a great addition to the ecosystem! Glad it's getting some new love!

@NeilsC
Copy link

NeilsC commented Mar 30, 2022

Just for a bit of context, I stumbled upon lograge because we're moving to Datadog for APM/logs and they highly recommend lograge to enable structured json logging in Rails. They also have this recommended solution for setting the log level based on the HTTP status of the operation. Their proposed solution looks very similar to what this PR does, so that's why I'm interested in getting it integrated as part of the gem.

Anyway, I'll take a stab at it.

@iloveitaly
Copy link
Collaborator

I wasn't ignoring you, but it has been three years without a reply since I created this PR, so I wasn't rushing either. :)

Ha, totally! Just trying to make sure folks in the community feel empowered to make changes and improve here :)

Thank you for your extended thoughts!

@d1rtyvans
Copy link

+1

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.

5 participants