[Messenger] deprecate LoggingMiddleware in favor of providing a logger to SendMessageMiddleware #30539
Conversation
253d5df to
f971d81
Compare
f971d81 to
30a2b22
Compare
There was a problem hiding this comment.
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.
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.
Here you are: the try/catch now wraps SendMessageMiddleware
This should address both your questions.
I 💯 disagree here :) Logging is a cross-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. |
30a2b22 to
480d30f
Compare
…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
480d30f to
24bdfd3
Compare
…r to SendMessageMiddleware
24bdfd3 to
2bff625
Compare
|
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
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.