Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Messenger] deprecate LoggingMiddleware in favor of providing a logger to SendMessageMiddleware#30539
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
253d5df tof971d81Compare
weaverryan left a comment
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.
Best viewed with?w=1 - looks perfect to me. Much easier to read and now I can see a bit more detail, like when the message is being sent, versus when it is received later and actually handled.
+1
ogizanagi left a comment
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'd be +1 for adding these specific log entries in send & handle middleware, but I'm not sure we have to deprecate and unwire the logger middleware from defaults.
It fits its purpose: general debug logs about message lifecycle in the bus and exception, independently of where the exception occurred in the bus (whereas you arbitrary chose to catch in send middleware here). It's also a simple & great showcase on how can be written and used a middleware.
| return$stack->next()->handle($envelope,$stack); | ||
| }catch (\Throwable$e) { | ||
| $context['exception'] =$e; | ||
| $this->logger->warning('An exception occurred while handling message "{class}"',$context); |
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.
Why catching and logging here instead of doing it in the HandleMessageMiddleware?
Also why catching & logging when handling, but not when sending?
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.
Here you are: the try/catch now wraps SendMessageMiddleware
This should address both your questions.
nicolas-grekas commentedMar 13, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I 💯 disagree here :) Logging is across-cutting concern. What this means is that it's a bad fit for a middleware. Keeping it as an example of middleware is spreading the idea that logging can be split in its own isolated layer. That's wrong. Because the LoggingMiddleware is a bad fit, its messages are of very low value. I don't want my logs to be filled with many uninformative log lines per messages. Thus the proposal to drop it. |
…d (nicolas-grekas)This PR was merged into the 4.3-dev branch.Discussion----------[Messenger] add welcome notice when running the command| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -The current behavior of `./bin/console messenger:consume-messages` is totally silent: you run it and nothing visible happens.Here is what is displayed with this PR:Combined with#30539, it gives:Commits-------673b58b [Messenger] add welcome notice when running the command
…r to SendMessageMiddleware
weaverryan commentedMar 14, 2019
Ping@ogizanagi ! |
…oviding a logger to SendMessageMiddleware (nicolas-grekas)This PR was merged into the 4.3-dev branch.Discussion----------[Messenger] deprecate LoggingMiddleware in favor of providing a logger to SendMessageMiddleware| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -Discussing with@simensen, we figured out the currently logged messages are not precise enough.Logging is a cross-cutting concern: splitting it in a dedicated middleware means losing details - or building complexity.Let's make things simple and log the best messages depending on the internal situation.While the component is experimental, removing the `LoggingMiddleware` altogether would break FrameworkBundle 4.2 when it is used with Messenger 4.3. Not worth the trouble when it's two lines to deprecate IMHO.Commits-------2bff625 [Messenger] deprecate LoggingMiddleware in favor of providing a logger to SendMessageMiddleware
Uh oh!
There was an error while loading.Please reload this page.
Discussing with@simensen, we figured out the currently logged messages are not precise enough.
Logging is a cross-cutting concern: splitting it in a dedicated middleware means losing details - or building complexity.
Let's make things simple and log the best messages depending on the internal situation.
While the component is experimental, removing the
LoggingMiddlewarealtogether would break FrameworkBundle 4.2 when it is used with Messenger 4.3. Not worth the trouble when it's two lines to deprecate IMHO.