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

[Messenger] deprecate LoggingMiddleware in favor of providing a logger to SendMessageMiddleware #30539

Merged
merged 1 commit into from Mar 15, 2019

Conversation

Projects
None yet
4 participants
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Mar 12, 2019

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.

@nicolas-grekas nicolas-grekas added this to the next milestone Mar 12, 2019

@nicolas-grekas nicolas-grekas requested a review from sroze as a code owner Mar 12, 2019

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:msg-logger branch 2 times, most recently from 253d5df to f971d81 Mar 12, 2019

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:msg-logger branch from f971d81 to 30a2b22 Mar 12, 2019

@weaverryan
Copy link
Member

weaverryan left a comment

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
Copy link
Member

ogizanagi left a 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);

This comment has been minimized.

@ogizanagi

ogizanagi Mar 13, 2019

Member

Why catching and logging here instead of doing it in the HandleMessageMiddleware?

Also why catching & logging when handling, but not when sending?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 13, 2019

Author Member

Here you are: the try/catch now wraps SendMessageMiddleware
This should address both your questions.

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Mar 13, 2019

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.

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.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:msg-logger branch from 30a2b22 to 480d30f Mar 13, 2019

nicolas-grekas added a commit that referenced this pull request Mar 13, 2019

feature #29303 [Messenger] add welcome notice when running the comman…
…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:
![image](https://user-images.githubusercontent.com/243674/54235039-af0a6c80-4510-11e9-89d8-3c1c55e946c0.png)

Combined with #30539, it gives:
![image](https://user-images.githubusercontent.com/243674/54235156-ed079080-4510-11e9-9d4d-9f27c87e16e5.png)

Commits
-------

673b58b [Messenger] add welcome notice when running the command

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:msg-logger branch from 480d30f to 24bdfd3 Mar 14, 2019

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:msg-logger branch from 24bdfd3 to 2bff625 Mar 14, 2019

@weaverryan

This comment has been minimized.

Copy link
Member

weaverryan commented Mar 14, 2019

Ping @ogizanagi !

@nicolas-grekas nicolas-grekas merged commit 2bff625 into symfony:master Mar 15, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Mar 15, 2019

feature #30539 [Messenger] deprecate LoggingMiddleware in favor of pr…
…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

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:msg-logger branch Mar 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.