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

community/feature-request#12 - Allow named logging channels #20079

Merged
merged 1 commit into from
Apr 16, 2021

Conversation

totten
Copy link
Member

@totten totten commented Apr 16, 2021

Overview

Make it easier to route log messages based on their topic (e.g. CiviContribute-related logs vs CiviMail-related logs).

This is a variation+extraction of @eileenmcnaughton's #20068.

Before

Civi::log() always returns the same instance of LoggerInterface, with no clear way to differentiate logs of different business subsystems.

After

Civi::log(...) allows you to optionally request a LoggerInterface for a specific theme, e.g.

Civi::log('mail')->error('Failed to connect to SMTP server');
Civi::log('ipn')->warning('Transaction rejected by payment processor');

Technical Details

A few things going on here:

  • Extensions may start using their own logs (Civi::log('myext')) without any special effort.
  • It is possible to replace or customize specific logs by defining a service log.CHANNEL_NAME.
  • The psr_log_manager is a service. An extension like https://lab.civicrm.org/extensions/monolog/ can replace the
    psr_log_manager and use the channel-name in its own way.

There is a consideration here in that the list of channels is open-ended. It will be impossible to (eg) detect that a log-user has made a typo in the channel-name. However, this seems like the better trade-off if the alternative is that extensions face races during installation/uninstallation.

Overview
----------------------------------------

Make it easier to route log messages based on their topic (e.g. CiviContribute-related logs vs CiviMail-related logs).

Before
------

`Civi::log()` always returns the same instance of `LoggerInterface`, with no
clear way to differentiate logs of different business subsystems.

After
-----

`Civi::log(...)` allows you to optionally request a `LoggerInterface` for a specific theme, e.g.

```php
Civi::log('mail')->error('Failed to connect to SMTP server');
Civi::log('ipn')->warning('Transaction rejected by payment processor');
```

Technical Details
-----------------

A few things going on here:

* Extensions may start using their own logs (`Civi::log('myext')`) without any special effort.
* It is possible to replace or customize specific logs by defining a service `log.CHANNEL_NAME`.
* The `psr_log_manager` is a service. An extension like https://lab.civicrm.org/extensions/monolog/
  can replace the `psr_log_manager` and use the channel-name in its own way.

There is a limitation here in that the list of channels is open-ended.  It
will be impossible to (eg) detect that a log-user has made a typo in the
channel-name.  However, this seems like the better trade-off if the
alternative is that extensions face races during
installation/uninstallation.
@civibot
Copy link

civibot bot commented Apr 16, 2021

(Standard links)

@civibot civibot bot added the master label Apr 16, 2021
@eileenmcnaughton
Copy link
Contributor

@totten this is great in terms of allowing extensions more options & I think is mergeable once I've done some testing.

On to shooting the breeze.....

  • in terms of imagining a core use case

'I want all ipn debug to go to a different file'

we can build on this (if we so choose, as a follow up) by

  1. switching calls in the ipn files in core to Civi::log('ipn')->debug('blah') - this would be no change unless an extension registers a logging service against that prefix

  2. if we wanted to log (by default in core) to a separate file we could do that by
    a) registering the logging service in core (which would still be overridable)
    b) configuring CRM_Core_Error_Log to accept a prefix & then making that part of the definition - such that it gets as far as this line

    CRM_Core_Error::debug_log_message($message, FALSE, '', $this->map[$level]);
    (the 3rd param is 'prefix' and determines what file it goes to)

@totten
Copy link
Member Author

totten commented Apr 16, 2021

  1. Yup, just sprinkle in the Civi:log('ipn') bit
  2. Passing through the channel-name for the debug_log_message(...$prefix...) would make it go to another file. At a minimum, one should do a little validation for well-formedness (ie a channel-name with /../ is bad).

@eileenmcnaughton
Copy link
Contributor

test fail seems unrelated but will re-run

@eileenmcnaughton
Copy link
Contributor

test this please

@eileenmcnaughton
Copy link
Contributor

OK - I tested this locally - if I do

  • Civi::log()->alert('blah') then it outputs to ConfigAndLog/default.log - this appears unchanged
  • Civi::log('my_log')->alert('blah') then it borks before & logs to default.lot after (improvement)
  • ^^ but in a hook I do
function my_log_civicrm_container($container) {
  $container->setDefinition('log.my_log', new Definition('\Civi\MonoLog\MonoLog', []))->setPublic(TRUE);
}

then it does nothing before but after it logs as configured in the extension

MOP

@seamuslee001 seamuslee001 merged commit 2fc171f into civicrm:master Apr 16, 2021
@artfulrobot
Copy link
Contributor

This is great, thanks @totten and eileen (no @ as I read you're taking a break)!

@totten totten deleted the master-logmgr branch April 17, 2021 18:11
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Apr 21, 2021
This builds on the recently added PR civicrm#20079
and changes the default behaviour such that if a channel is used, without any other intervention,
it will result in a change to the file name to reflect the channel

Channels are a new feature so this won't affect existing sites but
I think adding the prefix is a thing that people using different channels might
reasonably expect as a helpful default. I kept it such that we ignore anything
beyond a narrow range of characters - we could throw an exception
rather than silently ignore if we prefer
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Apr 21, 2021
This builds on the recently added PR civicrm#20079
and changes the default behaviour such that if a channel is used, without any other intervention,
it will result in a change to the file name to reflect the channel

Channels are a new feature so this won't affect existing sites but
I think adding the prefix is a thing that people using different channels might
reasonably expect as a helpful default. I kept it such that we ignore anything
beyond a narrow range of characters - we could throw an exception
rather than silently ignore if we prefer
erawat pushed a commit to compucorp/civicrm-core that referenced this pull request Sep 10, 2021
…channels commit

Include in CiviCRM 5.38

PR: civicrm#20079

Overview
----------------------------------------

Make it easier to route log messages based on their topic (e.g. CiviContribute-related logs vs CiviMail-related logs).

Before
------

`Civi::log()` always returns the same instance of `LoggerInterface`, with no
clear way to differentiate logs of different business subsystems.

After
-----

`Civi::log(...)` allows you to optionally request a `LoggerInterface` for a specific theme, e.g.

```php
Civi::log('mail')->error('Failed to connect to SMTP server');
Civi::log('ipn')->warning('Transaction rejected by payment processor');
```

Technical Details
-----------------

A few things going on here:

* Extensions may start using their own logs (`Civi::log('myext')`) without any special effort.
* It is possible to replace or customize specific logs by defining a service `log.CHANNEL_NAME`.
* The `psr_log_manager` is a service. An extension like https://lab.civicrm.org/extensions/monolog/
  can replace the `psr_log_manager` and use the channel-name in its own way.

There is a limitation here in that the list of channels is open-ended.  It
will be impossible to (eg) detect that a log-user has made a typo in the
channel-name.  However, this seems like the better trade-off if the
alternative is that extensions face races during
installation/uninstallation.
erawat pushed a commit to compucorp/civicrm-core that referenced this pull request Sep 10, 2021
…channels commit

Include in CiviCRM 5.38

PR: civicrm#20079

Overview
----------------------------------------

Make it easier to route log messages based on their topic (e.g. CiviContribute-related logs vs CiviMail-related logs).

Before
------

`Civi::log()` always returns the same instance of `LoggerInterface`, with no
clear way to differentiate logs of different business subsystems.

After
-----

`Civi::log(...)` allows you to optionally request a `LoggerInterface` for a specific theme, e.g.

```php
Civi::log('mail')->error('Failed to connect to SMTP server');
Civi::log('ipn')->warning('Transaction rejected by payment processor');
```

Technical Details
-----------------

A few things going on here:

* Extensions may start using their own logs (`Civi::log('myext')`) without any special effort.
* It is possible to replace or customize specific logs by defining a service `log.CHANNEL_NAME`.
* The `psr_log_manager` is a service. An extension like https://lab.civicrm.org/extensions/monolog/
  can replace the `psr_log_manager` and use the channel-name in its own way.

There is a limitation here in that the list of channels is open-ended.  It
will be impossible to (eg) detect that a log-user has made a typo in the
channel-name.  However, this seems like the better trade-off if the
alternative is that extensions face races during
installation/uninstallation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants