-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Dev json logs #22994
base: 5.x-dev
Are you sure you want to change the base?
Dev json logs #22994
Conversation
@krezreb please fix the PHPCS style check first. Referece: https://innocraft.atlassian.net/wiki/spaces/DE/pages/2395570181/Create+a+Github+action+to+check+if+code+formatting+is+as+per+PSR+standard |
@michalkleiner can you re-review this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works as expected, tested the standard logging, the exclude parameters and the custom function mechanism.
@sgiehl can you think of any edge cases here?
No issues with the PR itself, but just don't name any field |
Um in my testing this screen worked with json logging enabled. Could you share your ini settings please? |
Tweak information in the LogViewer so that the fields are still filled with relevant information
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requires a submodule update first, but otherwise this tested ok in the Log viewer area in Matomo as well as in the file output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introducing JSON logging sounds like a nice thing in general. But we maybe should do that in a more clean way and use more of the features we already have or monolog and/or DI allows us to.
|
||
'log.short.format' => Piwik\DI::factory(function (Container $c) { | ||
if ($c->has('ini.log.enable_json')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will actually also return true if enable_json = 0
is set in the config. Might be unexpected.
->constructorParameter('customFunctionFile', Piwik\DI::get('log.custom_function_file')) | ||
->constructorParameter('excludePatterns', Piwik\DI::get('log.exclude_patterns')), | ||
|
||
'log.custom_function_file' => Piwik\DI::factory(function (Container $c) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a specific reason why this is handled with a custom include file rather than using a LogProcessor class and configure using that LogProcessor by pre/appending it to log.processors
di config?
{ | ||
$this->logMessageFormat = $logMessageFormat; | ||
$this->allowInlineLineBreaks = $allowInlineLineBreaks; | ||
$this->excludePatterns = $excludePatterns; | ||
if ($customFunctionFile !== null) { | ||
$this->customFunction = include_once $customFunctionFile; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you checked if that class is ALWAYS instantiated only once per request? If that happens a second time include_once
will return true
, instead of the files content.
} | ||
} | ||
|
||
if ('json' === $this->logMessageFormat) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might have preferred having a dedicated JsonMessageFormatter
class (maybe inheriting from this class) and decide which class to use within the DI config. That might be more clean than having one class for all that.
->constructorParameter('allowInlineLineBreaks', false) | ||
->constructorParameter('customFunctionFile', Piwik\DI::get('log.custom_function_file')) | ||
->constructorParameter('excludePatterns', Piwik\DI::get('log.exclude_patterns')), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only setting the constructor parameters here actually only allows using such new "features" for file logging. Any reason why that shouldn't be available for e.g. database or screen logging?
Description:
Modifies the LineMessageFormatter to support outputting of log messages as json (one json object per line). This has a number of benefits for observability:
Also adds support for customers to add custom json processing to each line (useful for example for adding APM trace and span ids in for Datadog Unified service tagging users)
Review
Depends on plugin PR matomo-org/plugin-LogViewer#99
To test json_logs
config/config.ini.php
setenable_json = true
(see example below)index.php?module=LogViewer
logs should appear correctly in the interfaceTo test exclude_pattens
config/config.ini.php
set one or more exclude_patterns, separated by pipes(see example below)To test custom_function_file
config/config.ini.php
specify custom_function_file (see example below)custom.php