Add a public API for broadcasting logs#48615
Conversation
There was a problem hiding this comment.
Ideally would have preferred to be able to do BroadcastLogger.new(logger1, logger2), but that would mean either:
- Modify the original signature
- Create a PORO and implement duck typing because I felt the BroadcastLogger should behave just like a logger.
Not sure what's the best option
There was a problem hiding this comment.
I think we should do the latter.
There was a problem hiding this comment.
Sounds good thanks. So I guess if there is no red flag on this implementation, I'll go ahead and finish the PR (the AS test suite is passing already, I don't think this PR will change a lot to have a green CI).
Regarding
Broadcasting is in a grey zone and not referenced in our docs but I saw it often in apps and libraries. This refactor would break them.
Should we go through a deprecation cycle or nop?
There was a problem hiding this comment.
nop. The broadcast method is nodoced.
I have no reservations about this implementation. I think it will be way easier to use and maintain.
7fc578f to
77de8a6
Compare
2f0d5cf to
e3fc8f8
Compare
|
Originally wanted to fix TaggedLogging and implement a better broadcast logger at the same time (because they are kind of coupled and the issues occur with tagged logging when used with a broadcast), but the changes are too big for a single PR and the the many diff hunks would be annoying to review. I changed this PR to focus only on broadcasting and will propose the change for tagged logging in another PR. |
There was a problem hiding this comment.
Not sure what's the best approach here.
In the context of a broadcast, getting the level (and the progname above) don't make sense because the level on each loggers that are part of the broadcast can be different from one another.
In the previous implementation, level would return the level of the first logger in the broadcast. So I think we have a few options:
- Do the same as before and return the level of the first logger.
- Always return nil
- Don't implement the method.
There was a problem hiding this comment.
Maybe return the higher level between all the loggers that are going to be brodcasted to?
There was a problem hiding this comment.
That's a good idea 👍
There was a problem hiding this comment.
Maybe return the higher level between all the loggers that are going to be brodcasted to?
There was a problem hiding this comment.
Maybe return something like "Broadcast"?
There was a problem hiding this comment.
Rails doesn't use yard, so let's change the documentation to follow Rails conventions.
There was a problem hiding this comment.
Do we want to allow assigning the progname? I think all the loggers inside the broadcast will have different prog names and we don't want to make all of them the same.
There was a problem hiding this comment.
Yeah I agree that assigning the progname for all loggers is weird. But I was thinking that the behaviour on this broadcast should make it clear that any methods called on it will modify all inner loggers. Could be confusing if progname= is the only exception to that rule?
No strong opinion though and I'm happy to change it.
There was a problem hiding this comment.
Good point. But I think progname is really uniq for each logger so I think it would be better to now allow a mass update. We can even raise an exception. But given progname will return Boardcast make this method change only the progname of the broadcast?
f2924c6 to
cf65083
Compare
|
@Edouard-chin Could you squash your commits and rebase the changelog please? 🙏 |
|
👍 Thanks for marking this for the 7.1 release. I’ll do tomorrow when I
come back home !
Le dim. 24 sept. 2023 à 00:51, zzak ***@***.***> a écrit :
… @Edouard-chin <https://github.com/Edouard-chin> Could you squash your
commits and rebase the changelog please? 🙏
—
Reply to this email directly, view it on GitHub
<#48615 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB567BWASQXNAWTCYA5NKTDX35RXDANCNFSM6AAAAAAZZIUWQ4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
cf65083 to
54f38bf
Compare
|
Should be good for another 👀 |
- ## Context While working on rails#44695, I realised that Broadcasting was still a private API, although it’s commonly used. Rafael mentioned that making it public would require some refactor because of the original implementation which was hard to understand and maintain. ### Changing how broadcasting works: Broadcasting in a nutshell worked by “transforming” an existing logger into a broadcasted one. The logger would then be responsible to log and format its own messages as well as passing the message along to other logger it broadcasts to. The problem with this approach was the following: - Heavy use of metaprogramming. - Accessing the loggers in the broadcast wasn’t possible. Removing a logger from the broadcast either. - More importantly, modifying the main logger (the one that broadcasts logs to the others) wasn’t possible and the main source of misunderstanding. ```ruby logger = Logger.new(STDOUT) stderr_logger = Logger.new(STDER)) logger.extend(AS::Logger.broadcast(stderr_logger)) logger.level = DEBUG # This modifies the level on all other loggers logger.formatter = … # Modified the formatter on all other loggers ``` To keep the contract unchanged on what Rails.logger returns, the new BroadcastLogger class implement duck typing with all methods that has the vanilla Ruby Logger class. It's a simple and boring PORO that keeps an array of all the loggers that are part of the broadcast and iterate over whenever a log is sent. Now, users can access all loggers inside the broadcast and modify them on the fly. They can also remove any logger from the broadcast at any time. ```ruby # Before stdout_logger = Logger.new(STDOUT) stderr_logger = Logger.new(STDER) file_logger = Logger.new(“development.log”) stdout_logger.extend(AS::Logger.broadcast(stderr_logger)) stdout_logger.extend(AS::Logger.broadcast(file_logger)) # After broadcast = BroadcastLogger.new(stdout_logger, stderr_logger, file_logger) ``` I also think that now, it should be more clear for users that the broadcast sole job is to pass everything to the whole loggers in the broadcast. So there should be no surprise that all loggers in the broadcast get their level modified when they call `broadcast.level = DEBUG` . It’s also easier to wrap your head around more complex setup such as broadcasting logs to another broadcast: `broadcast.broadcast_to(stdout_logger, other_broadcast)`
54f38bf to
1fbd812
Compare
|
I'd like to write a proper issue/PR but I'm not sure how to debug further regarding logging changes. Logs worked fine in 7.1.0.beta1 but in 7.1.0.rc1 (with this in) I no longer have most logs in stdout, only in development.log. I only have I tried creating a fresh rails app with a generator, and the logs showing in stdout there, as expected. I've looked at app and environment configs and can't see any diff there. I've put a debugger inside LogSubscriber but it looks the same for both apps. So I'm not sure what's going on and how to debug it further 🤔 Here's the app: https://github.com/miharekar/decent-visualizer/tree/broken-logs |
|
Thanks for letting me know, you are right I'm seeing the same thing. Sorry for missing that. Looking now 👀 |
|
Opened a PR in #49417 to fix this. |
- An oversight of rails#48615 is that it changes the `Rails.logger` to be a broadcast logger after the app is booted. Anything referencing `Rails.logger` during the boot process will get a simple logger and ultimately resulting in logs not being broadcasted. For example `ActionController::Base.logger.info("abc")` would just output logs in the `development.log` file, not on STDOUT. ---- The only solution I could think of is to create a BroadcastLogger earlier at boot, and add logger to that broadcast when needed (instead of modiyfing the `Rails.logger` variable).
In rails/rails#48615, Rails 7.1 introduced a new BroadcastLogger class that allows users to send logs to multiple loggers, which means we need to adjust the SDK's logger assignment for it.
In rails/rails#48615, Rails 7.1 introduced a new BroadcastLogger class that allows users to send logs to multiple loggers, which means we need to adjust the SDK's logger assignment for it.
In rails/rails#48615, Rails 7.1 introduced a new BroadcastLogger class that allows users to send logs to multiple loggers, which means we need to adjust the SDK's logger assignment for it.
* Adopt Rails 7.1's new BroadcastLogger In rails/rails#48615, Rails 7.1 introduced a new BroadcastLogger class that allows users to send logs to multiple loggers, which means we need to adjust the SDK's logger assignment for it. * Update changelog
Rails 7.1.0 introduces a new default logger class, ActiveSupport::BroadcastLogger, which does not inherit from Logger. (See: rails/rails#48615.) Instead of testing for a specific class, we can test the interface.
- An oversight of rails#48615 is that it changes the `Rails.logger` to be a broadcast logger after the app is booted. Anything referencing `Rails.logger` during the boot process will get a simple logger and ultimately resulting in logs not being broadcasted. For example `ActionController::Base.logger.info("abc")` would just output logs in the `development.log` file, not on STDOUT. ---- The only solution I could think of is to create a BroadcastLogger earlier at boot, and add logger to that broadcast when needed (instead of modiyfing the `Rails.logger` variable).
Rails 7.1.0 introduces a new default logger class, ActiveSupport::BroadcastLogger, which does not inherit from Logger. (See: rails/rails#48615.) Instead of testing for a specific class, we can test the interface.
Rails 7.1.0 introduces a new default logger class, ActiveSupport::BroadcastLogger, which does not inherit from Logger. (See: rails/rails#48615.) Instead of testing for a specific class, we can test the interface.
Rails 7.1.0 introduces a new default logger class, ActiveSupport::BroadcastLogger, which does not inherit from Logger. (See: rails/rails#48615.) Instead of testing for a specific class, we can test the interface. Co-authored-by: zogoo <ch.tsogbadrakh@gmail.com>
Context
While working on #44695, I realised that Broadcasting was still a private API, although it’s commonly used. @rafaelfranca mentioned that making it public would require some refactor because of the original implementation which was hard
to understand and maintain.
Broadcasting
Broadcasting in a nutshell worked by “transforming” an existing logger into a broadcasted one.
The logger would then be responsible to log and format its own messages as well as passing the message along to other logger it broadcasts to.
The problem with this approach was the following:
Heavy use of metaprogramming.
Accessing the loggers in the broadcast wasn’t possible. Removing a logger from the broadcast either.
More importantly, modifying the main logger (the one that broadcasts logs to the others) wasn’t possible and the main source of misunderstanding.
To keep the contract unchanged on what
Rails.loggerreturns, the newBroadcastLoggerclass implement duck typing with all methods that has the vanilla Ruby Logger class.It's a simple and boring PORO that keeps an array of all the loggers that are part of the broadcast and iterate over whenever a log is sent.
Now, users can access all loggers inside the broadcast and modify them on the fly. They can also remove any logger from the broadcast at any time.
I also think that now, it should be more clear for users that the broadcast sole job is to pass everything to the whole loggers in the broadcast. So there should be no surprise that all loggers in the broadcast get their level modified when they call
broadcast.level = DEBUG.It’s also easier to wrap your head around more complex setup such as broadcasting logs to another broadcast:
broadcast.broadcast_to(stdout_logger, other_broadcast)