-
Notifications
You must be signed in to change notification settings - Fork 22k
Make framework log subscribers to consume structured events #55900
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
Conversation
d1d2f2e to
1bec302
Compare
609b1f4 to
8b810c8
Compare
…seful for logging Related to #55900.
|
Shiped #55904 before this one. |
cecbd80 to
3493a2b
Compare
| CYAN = "\e[36m" | ||
| WHITE = "\e[37m" | ||
|
|
||
| mattr_accessor :colorize_logging, default: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this breaking API change?
Since mattr_accessor are not picked up by RDoc, it is not visible in the public API docs however:
https://api.rubyonrails.org/classes/ActiveSupport/LogSubscriber.html
I did find one case of it in the wild, when running rails_semantic_logger tests against this branch:
amazing-print/amazing_print@main...zzak:amazing_print:rails-log-subscribers-colorize_logging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it wasn't because it wasn't visible in docs. I would opt to have private API users update their own code, but I'm open to suggestions. If someone else agrees, we can add deprecations, but I don't think it is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it wasn't because it wasn't visible in docs.
This is the general rule, yes so I think we're fine. The public API which is documented is through configuration:
https://edgeapi.rubyonrails.org/classes/Rails/Application/Configuration.html#method-i-colorize_logging
But I think that using mattr_* is intentionally meant to be private is false, that is just a side-effect of missing RDoc support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks Rails projects that use Awesome Print (or its maintained fork Amazing Print):
# lib/amazing_print/ext/active_support.rb
if defined?(ActiveSupport::LogSubscriber)
AwesomePrint.force_colors! ActiveSupport::LogSubscriber.colorize_logging
endWhile using the public ActiveSupport.colorize_logging API would be a quick fix for Amazing Print (I recently contributed there and the maintainers are responsive), Awesome Print has been stale for years and many projects probably still depend on it. So upgrading to Rails 8.2+ would mean requiring companies to switch to amazing_print.
I understand this is possibly a private API, but are we okay with proceeding as is? Or is this possibly private API used be enough "high profile" gems to warrant a deprecation or a mention in the upgrade guides?
The most "high profiles" result I see from https://sourcegraph.com/search?q=context:global+%22ActiveSupport::LogSubscriber.colorize_logging%22&patternType=keyword&sm=0 are awesome print, amazing print, a profiler in the gitlab repo.
|
Tried this branch with If you unsubscribe from these do other things break, either intentionally or unintentionally. 🤔 Lograge also does something like this: Not sure how actionable it is, just came to me when thinking about this and thought it might be valuable context. 🙏 |
3493a2b to
2117e11
Compare
Most log subscribers are nodoc. I made all log subscribers nodoc in #55755, and if that is released in 8.1, and this in 8.2, that should be sufficient IMO. |
|
I'll backport the more recent structured logging additions here to 8.1 stable so that we can include that in the full release (or another RC). Looks like the pressure is off to get this out before 8.1, and its a pretty big change, so I'll leave it open for a few more days for feedback. |
activesupport/lib/active_support/event_reporter/log_subscriber.rb
Outdated
Show resolved
Hide resolved
5b73cee to
4f9b7d3
Compare
4f9b7d3 to
b601abc
Compare
activesupport/lib/active_support/event_reporter/log_subscriber.rb
Outdated
Show resolved
Hide resolved
b601abc to
d8977f2
Compare
Introduce base class for structured event logging. Because so much of the logic is copied from the original log subscriber, this also introduced ActiveSupport::ColorizeLogging that may be used to log in color.
d8977f2 to
b646f35
Compare
|
|
||
| module ActiveSupport | ||
| class EventReporter | ||
| class LogSubscriber |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there is a CHANGELOG entry for this class, should we add some rdoc documentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'll put this on my TODO list, but PRs are welcome in the meantime!
Motivation / Background
This Pull Request has been created because framework log subscribers currently consume notification events. Since the introduction of structured event subscribers in #55690, this doesn't necessarily need to happen anymore. We can instead use the structured event subscribers to produce logs instead for environments like development.
Detail
This Pull Request adds a base class for framework structured event log subscribers, and changes current log subscribers (private API since #55755) to consume structured events instead of notification events.
Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]