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

[MonologBridge] Add monolog processors adding route and command info #28330

Merged
merged 1 commit into from Mar 17, 2019

Conversation

Projects
None yet
5 participants
@trakos
Copy link
Contributor

commented Aug 31, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR symfony/symfony-docs#10244

This PR adds two simple processors that add context to every log entry.

RouteProcessor adds routing information:
app.INFO: Some log text {"someContext":"ctx"} {"route":{"controller":"App\\Controller\\SomeController::number","route":"index","route_params":[]}

ConsoleCommandProcessors adds current command information:
app.INFO: Some log text {"someContext":"ctx"} {"command":{"name":"app:some-command","arguments":{"command":"app:some-command","some-argument":10}}}

For ConsoleCommandProcessor I've decided against including command options by default, because there's a lot of default ones:
"options":{"help":false,"quiet":false,"verbose":false,"version":false,"ansi":false,"no-ansi":false,"no-interaction":false,"env":"dev","no-debug":false}. This behavior can be changed with a constructor argument.

@lyrixx

This comment has been minimized.

Copy link
Member

commented Sep 3, 2018

I like it but I have some comments

  1. The RouteProcessor does not work well. The "RouteData" should be unset on "kernel.finish_request".
  2. The RoutePRocessor might be merged with the https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Monolog/Processor/WebProcessor.php ?
@trakos

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2018

  1. Yes, you're right, I didn't consider subrequests. I've updated it - I've made it so that it includes an array of (sub)requests, kind of like a stacktrace:
[
    {
        "controller": "App\\Controller\\SomeController::number",
        "route": "index",
        "route_params": []
    },
    {
        "controller": "App\\Controller\\SomeController::other",
        "route": "other",
        "route_params": {
            "page": "235"
        }
    }
]

The last one is the current one. It pops last entry during "kernel.finish_request". What do you think?

  1. I can combine them if you prefer. There's some things to consider:

    • Users won't be aware that update will make their log messages longer
    • WebProcessor is an extend of monolog's WebProcessor, I think it's supposed to add the same information to the log
    • There's probably some users that enable WebProcessor just to have IP logged and won't be happy about lengthier messages

@lyrixx What do you think?

@trakos trakos force-pushed the trakos:monolog_processors_command_and_route branch 2 times, most recently from e6dee3f to 803dc25 Sep 3, 2018

@fabpot

This comment has been minimized.

Copy link
Member

commented Oct 10, 2018

@lyrixx Can you review the latest version of this PR? Thank you.

@lyrixx
Copy link
Member

left a comment

It seems better. 👏
I left some comments.
Thanks for your work.

@trakos trakos force-pushed the trakos:monolog_processors_command_and_route branch from 329933c to 228511e Oct 12, 2018

@trakos

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2018

Thanks for your feedback @lyrixx . I've updated the changes.

In RouteProcessor I've decided to use spl_object_id instead of SplObjectStorage, there's less code that way. Is that OK?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Nov 10, 2018

meanwhile, ProcessorInterface has been removed from the bridge and added to Monolog directly in Seldaek/monolog#1204
If we don't want to bump the minimum version of Monolog, we should register the new classes for autoconfiguration, see symfony/monolog-bundle#285

@trakos

This comment has been minimized.

Copy link
Contributor Author

commented Nov 10, 2018

Would you like me to create PR in symfony/monolog-bundle registering this classes, or should I wait till some decision is made?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Nov 10, 2018

Let's first finish this one :)

@lyrixx
Copy link
Member

left a comment

LGTM except one tiny comment

Show resolved Hide resolved src/Symfony/Bridge/Monolog/CHANGELOG.md Outdated

@trakos trakos force-pushed the trakos:monolog_processors_command_and_route branch 3 times, most recently from ac92e84 to d331569 Nov 13, 2018

@trakos

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2018

@lyrixx thanks, I've made the change. I've also removed extending ProcessorInterface, since it's no longer there. It should be ready now.

@lyrixx

lyrixx approved these changes Mar 16, 2019

@lyrixx

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

let's merge this one (except for native English, I can not tell)

@fabpot

fabpot approved these changes Mar 17, 2019

@fabpot fabpot force-pushed the trakos:monolog_processors_command_and_route branch from e17ae9a to 669f6b2 Mar 17, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 17, 2019

Thank you @trakos.

@fabpot fabpot merged commit 669f6b2 into symfony:master Mar 17, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 17, 2019

feature #28330 [MonologBridge] Add monolog processors adding route an…
…d command info (trakos)

This PR was squashed before being merged into the 4.3-dev branch (closes #28330).

Discussion
----------

[MonologBridge] Add monolog processors adding route and command info

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        | symfony/symfony-docs#10244

This PR adds two simple processors that add context to every log entry.

RouteProcessor adds routing information:
`app.INFO: Some log text {"someContext":"ctx"} {"route":{"controller":"App\\Controller\\SomeController::number","route":"index","route_params":[]}`

ConsoleCommandProcessors adds current command information:
`app.INFO: Some log text {"someContext":"ctx"} {"command":{"name":"app:some-command","arguments":{"command":"app:some-command","some-argument":10}}}`

For ConsoleCommandProcessor I've decided against including command options by default, because there's a lot of default ones:
`"options":{"help":false,"quiet":false,"verbose":false,"version":false,"ansi":false,"no-ansi":false,"no-interaction":false,"env":"dev","no-debug":false}`. This behavior can be changed with a constructor argument.

Commits
-------

669f6b2 [MonologBridge] Add monolog processors adding route and command info

wouterj added a commit to symfony/symfony-docs that referenced this pull request Apr 9, 2019

feature #10244 Add documentation on new monolog processors (trakos)
This PR was merged into the master branch.

Discussion
----------

Add documentation on new monolog processors

Documentation supporting symfony/symfony#28330

Commits
-------

3421c6c [MonologBridge] Add documentation on new processors

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

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.