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

[HttpKernel][FrameworkBundle] Add a minimalist default PSR-3 logger #24300

Closed
wants to merge 4 commits into
base: 3.4
from

Conversation

@dunglas
Member

dunglas commented Sep 23, 2017

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

This PR provides a minimalist PSR-3 logger that is always available when FrameworkBundle is installed.
By default, it writes errors on stderr, regular logs on stdout and discards debug data (this is configurable).

This approach has several benefits:

  • It's what expect from an app logging systems of major containerization and orchestration tools including Docker and Kubernetes, as well as most cloud providers such as Heroku and Google Container Engine. If the app follows this standard (and it's not currently the case with Symfony by default) logs will be automatically collected, aggregated and stored.
  • It's in sync with the "back to Unix roots" philosophy of Flex
  • Logs are directly displayed in the console when running the integrated PHP web server (bin/console server:start or Flex's make serve), Create React App also do that for instance.
  • It fixes a common problem when installing Flex recipes: many bundles expect a logger service but currently there is none available by default, and you usually get a "logger" service not found error (because packages depend of the PSR, but the PSR doesn't provide a logger service).
@ogizanagi

So, this PR aims to provide a sensible default logger, when not using MonologBundle, for instance when starting a simple project with flex.
I like it :)

I wonder: would it be interesting to allow not interpolating messages, but inline context at the end (see Monolog's LineFormatter)? It'll allow better aggregation, but I don't know if the logging systems you mentioned could leverage it easily.

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Sep 24, 2017

Member

I've changed the way levels are handled:

  • Only stderr is used by default
  • Levels less critical than warning are ignored by default
  • if kernel.debug is set, all levels enabled (in the compiled pass, and sent to stderr)

I've also fixed most comments, thanks everybody for the review.

Member

dunglas commented Sep 24, 2017

I've changed the way levels are handled:

  • Only stderr is used by default
  • Levels less critical than warning are ignored by default
  • if kernel.debug is set, all levels enabled (in the compiled pass, and sent to stderr)

I've also fixed most comments, thanks everybody for the review.

@nicolas-grekas

the logger should have a "reset" method, and be tagged "kernel.reset".

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Sep 24, 2017

Member

@ogizanagi

I wonder: would it be interesting to allow not interpolating messages, but inline context at the end (see Monolog's LineFormatter)? It'll allow better aggregation, but I don't know if the logging systems you mentioned could leverage it easily.

AFAIK, most platforms doesn't support that unless you use their proprietary API (ex for Stackdriver: https://cloud.google.com/logging/docs/reference/v2/rest/v2/LogEntry).
I'm not aware of a standard way to do that (but if you have a hint, I'm very interested).

IMO we should keep this builtin logger as small and standard as possible and recommend to use other libraries such as Monolog for more advanced uses like this one... until a standard appears.

Member

dunglas commented Sep 24, 2017

@ogizanagi

I wonder: would it be interesting to allow not interpolating messages, but inline context at the end (see Monolog's LineFormatter)? It'll allow better aggregation, but I don't know if the logging systems you mentioned could leverage it easily.

AFAIK, most platforms doesn't support that unless you use their proprietary API (ex for Stackdriver: https://cloud.google.com/logging/docs/reference/v2/rest/v2/LogEntry).
I'm not aware of a standard way to do that (but if you have a hint, I'm very interested).

IMO we should keep this builtin logger as small and standard as possible and recommend to use other libraries such as Monolog for more advanced uses like this one... until a standard appears.

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Sep 26, 2017

Member

Changed the implementation to:

  • Only allow a minimal log level
  • Only allow one input (as in Go)
Member

dunglas commented Sep 26, 2017

Changed the implementation to:

  • Only allow a minimal log level
  • Only allow one input (as in Go)
Show outdated Hide outdated src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php
Show outdated Hide outdated src/Symfony/Component/HttpKernel/Log/Logger.php
Show outdated Hide outdated src/Symfony/Component/HttpKernel/Log/Logger.php
/**
* @param string $level
* @param string $message

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Sep 28, 2017

Member

missing $context

@nicolas-grekas

nicolas-grekas Sep 28, 2017

Member

missing $context

This comment has been minimized.

@dunglas

dunglas Sep 28, 2017

Member

On purpose, the type is already documented by the typehint.

@dunglas

dunglas Sep 28, 2017

Member

On purpose, the type is already documented by the typehint.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Sep 28, 2017

Member

when I did so on some of my PRs, stof told me we don't do partial doc blocks :)

@nicolas-grekas

nicolas-grekas Sep 28, 2017

Member

when I did so on some of my PRs, stof told me we don't do partial doc blocks :)

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Sep 28, 2017

Member

but we ma also remove the docblock entirely :)

@nicolas-grekas

nicolas-grekas Sep 28, 2017

Member

but we ma also remove the docblock entirely :)

This comment has been minimized.

@dunglas

dunglas Sep 28, 2017

Member

Other types aren't inferable.

@dunglas

dunglas Sep 28, 2017

Member

Other types aren't inferable.

@fabpot fabpot dismissed their stale review Sep 28, 2017

fixed

@nicolas-grekas

👍 as is :)

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 28, 2017

Member

when test are reasonably green :)

Member

nicolas-grekas commented Sep 28, 2017

when test are reasonably green :)

@fabpot

fabpot approved these changes Sep 28, 2017

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Sep 29, 2017

Member

Thank you @dunglas

Member

fabpot commented Sep 29, 2017

Thank you @dunglas

fabpot added a commit that referenced this pull request Sep 29, 2017

feature #24300 [HttpKernel][FrameworkBundle] Add a minimalist default…
… PSR-3 logger (dunglas)

This PR was squashed before being merged into the 3.4 branch (closes #24300).

Discussion
----------

[HttpKernel][FrameworkBundle] Add a minimalist default PSR-3 logger

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | yes <!-- don't forget updating src/**/CHANGELOG.md files -->
| BC breaks?    | no
| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

This PR provides a minimalist PSR-3 logger that is always available when FrameworkBundle is installed.
By default, it writes errors on `stderr`, regular logs on `stdout` and discards debug data (this is configurable).

This approach has several benefits:

- It's what expect from an app logging systems of major containerization and orchestration tools including [Docker](https://docs.docker.com/engine/admin/logging/view_container_logs/) and [Kubernetes](https://kubernetes.io/docs/concepts/cluster-administration/logging/), as well as most cloud providers such as [Heroku](https://devcenter.heroku.com/articles/logging#writing-to-your-log) and [Google Container Engine](https://kubernetes.io/docs/tasks/debug-application-cluster/logging-stackdriver/). If the app follows this standard (and it's not currently the case with Symfony by default) logs will be automatically collected, aggregated and stored.
- It's in sync with the "back to Unix roots" philosophy of Flex
- Logs are directly displayed in the console when running the integrated PHP web server (`bin/console server:start` or Flex's `make serve`), Create React App also do that for instance.
- It fixes a common problem when installing Flex recipes: many bundles expect a logger service but currently there is none available by default, and you usually get a `"logger" service not found error` (because packages depend of the PSR, but the PSR doesn't provide a logger service).

Commits
-------

9a06513 [HttpKernel][FrameworkBundle] Add a minimalist default PSR-3 logger

@fabpot fabpot closed this Sep 29, 2017

fabpot added a commit that referenced this pull request Sep 30, 2017

minor #24385 [FrameworkBundle] Register a NullLogger from test kernel…
…s (ogizanagi)

This PR was merged into the 3.4 branch.

Discussion
----------

[FrameworkBundle] Register a NullLogger from test kernels

| Q             | A
| ------------- | ---
| Branch?       | 3.4 <!-- see comment below -->
| Bug fix?      | no
| New feature?  | no <!-- don't forget updating src/**/CHANGELOG.md files -->
| BC breaks?    | no
| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->
| Tests pass?   | yes
| Fixed tickets | N/A <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | N/A

Relates to #24300

This will avoid unnecessary output on Travis or when running FrameworkBundle tests locally:
- before: https://travis-ci.org/symfony/symfony/jobs/281624658#L3594-L3635
- after: https://travis-ci.org/symfony/symfony/jobs/281643868#L3599-L3617

but also needed for anyone running functional tests on their project and using the default logger, in order to not get garbage output.

Do we need to find a more generic solution (like exposing a `framework.default_logger` option so users can set it to false for test)? Or just documenting this?

Commits
-------

c109dcd [FrameworkBundle] Register a NullLogger from test kernels

This was referenced Oct 18, 2017

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Sep 19, 2018

minor #10321 Minimalist default PSR-3 logger (dunglas)
This PR was squashed before being merged into the 3.4 branch (closes #10321).

Discussion
----------

Minimalist default PSR-3 logger

symfony/symfony#24300

Commits
-------

da84567 Minimalist default PSR-3 logger
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment