Dev json logs #22994

krezreb · 2025-01-28T14:51:36Z

Description:

Please include a description of this change and which issue it fixes. If no issue exists yet please include context and what problem it solves.

Review

Depends on plugin PR matomo-org/plugin-LogViewer#99

To test json_logs

Checkout the dev-json-logs branch on Matomo core
Modify config/config.ini.php set the format as json in (see example below)

[log]
log_writers[] = file
log_level = DEBUG
enable_json = true
logger_file_path = tmp/logs/matomo.log

Tail the the log file and you should see the logs as json
Check the logviewer plugin index.php?module=LogViewer logs should appear correctly in the interface

To test exclude_pattens

Modify config/config.ini.php set one or more exclude_patterns, separated by pipes(see example below)

[log]
log_writers[] = file
log_level = DEBUG
exclude_patterns =  "Piwik\Plugin\Manager|Matomo is not set up"
enable_json = true
logger_file_path = tmp/logs/matomo.log

Tail the the log file and you should see the logs as json, EXCEPT FOR logs matching the exclude patterns

To test custom_function_file

Modify config/config.ini.php specify custom_function_file (see example below)

[log]
log_writers[] = file
log_level = DEBUG
custom_function_file = /var/www/html/config/custom.php
enable_json = true
logger_file_path = tmp/logs/matomo.log

Example custom.php

<?php

function userHandler(array $array) : ?array
{
        $array["foo"] = "bar";
        return $array;
};
# return a string mathcing the name of the above function
return 'userHandler';

Tail the the log file and you should see the logs as json with the above custom logic run, in the above case a key "foo" will be set to the value "bar"

…terns

patrickli · 2025-01-28T20:03:54Z

@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 · 2025-01-28T21:44:13Z

plugins/Monolog/Formatter/LineMessageFormatter.php

    {
        $this->logMessageFormat = $logMessageFormat;
        $this->allowInlineLineBreaks = $allowInlineLineBreaks;
+        $this->excludePatterns = $excludePatterns;
+        if (gettype($customFunctionFile) === "string") {


you can add typehint to the constructor param and don't need to check for it being a string, just not empty.

michalkleiner · 2025-01-28T21:44:30Z

plugins/Monolog/Formatter/LineMessageFormatter.php

    private $allowInlineLineBreaks;

    /**
     * @param string $logMessageFormat
     * @param bool $allowInlineLineBreaks If disabled, a log message will be created for each line
     */
-    public function __construct($logMessageFormat, $allowInlineLineBreaks = true)
+    public function __construct($logMessageFormat, $allowInlineLineBreaks = true, $excludePatterns = null, $customFunctionFile = null)


Suggested change

public function __construct($logMessageFormat, $allowInlineLineBreaks = true, $excludePatterns = null, $customFunctionFile = null)

public function __construct($logMessageFormat, $allowInlineLineBreaks = true, array $excludePatterns = null, string $customFunctionFile = null)

michalkleiner · 2025-01-28T21:45:05Z

plugins/Monolog/Formatter/LineMessageFormatter.php

@@ -43,6 +48,21 @@ public function format(array $record)

        $message = trim($record['message']);

+        if (gettype($this->excludePatterns) === "array") {


with the constructor mandated type of the param, this can just be

Suggested change

if (gettype($this->excludePatterns) === "array") {

if (!empty($this->excludePatterns)) {

michalkleiner · 2025-01-28T21:45:40Z

plugins/Monolog/Formatter/LineMessageFormatter.php

+            }
+        }
+
+        if ($this->logMessageFormat == 'json') {


might be a tad safer

Suggested change

if ($this->logMessageFormat == 'json') {

if ('json' === $this->logMessageFormat) {

I agree about the === but is there a reason to put json first? legibility?

It's the Yoda condition style, in general preventing situations when someone forgets an equal sign and accidentally assigns the value to the variable instead of doing a comparison. With the string or number first that can't happen. It's not a strict thing we check for in CS yet but might be at some point.

@michalkleiner hmm..... first time I've seen this, it is. Modify the line, I will

michalkleiner · 2025-01-28T21:46:05Z

plugins/Monolog/Formatter/LineMessageFormatter.php

@@ -62,6 +82,27 @@ public function format(array $record)
        return $total;
    }

+    private function jsonMessage($class, $message, $date, $record)


can we add typehints to the params and a return type?

michalkleiner · 2025-01-28T21:46:44Z

plugins/Monolog/Formatter/LineMessageFormatter.php

+            "message" => $message,
+            "level" => $record['level_name'],
+            "trace" => $trace,
+            "requestId" => $requestId


a trailing comma is preferred in case a new item is added to minimise git diff on unnecessary lines

Suggested change

"requestId" => $requestId

"requestId" => $requestId,

michalkleiner · 2025-01-28T21:47:28Z

plugins/Monolog/Formatter/LineMessageFormatter.php

+            "requestId" => $requestId
+        ];
+
+        if (gettype($this->customFunction) === "string") {


this might check for not being empty and also if it's actually callable

Suggested change

if (gettype($this->customFunction) === "string") {

if (is_callable($this->customFunction)) {

michalkleiner · 2025-01-28T21:48:25Z

plugins/Monolog/config/config.php

+            $xcl = [];
+            foreach (explode("|", $c->get('ini.log.exclude_patterns')) as $p) {
+                $xcl[]  = trim($p);
+            }
+            return $xcl;


Suggested change

$xcl = [];

foreach (explode("|", $c->get('ini.log.exclude_patterns')) as $p) {

$xcl[] = trim($p);

}

return $xcl;

$excl = [];

foreach (explode('|', $c->get('ini.log.exclude_patterns')) as $p) {

$excl[] = trim($p);

}

return $excl;

michalkleiner · 2025-01-28T21:49:22Z

plugins/Monolog/config/config.php

+            $path = $c->get('ini.log.custom_function_file');
+            if (!file_exists($path)) {
+                return null;
+                throw new \Exception('Specified path to custom_function_file does not exist: ' . $path);


Wont' be thrown if we return above it. Similar for the readability check.

@michalkleiner oh yeah that was test code I forgot to go back over. What do you think / what is our general philosophy in this case? Should the code fail silently or throw an exception and break logging?

michalkleiner · 2025-01-28T21:50:14Z

plugins/Monolog/config/config.php

+    'log.exclude_patterns' => Piwik\DI::factory(function (Container $c) {
+        if ($c->has('ini.log.exclude_patterns')) {
+            $xcl = [];
+            foreach (explode("|", $c->get('ini.log.exclude_patterns')) as $p) {
+                $xcl[]  = trim($p);
+            }
+            return $xcl;
+        }
+        return null;
+    }),


seems like a duplicated section

Suggested change

'log.exclude_patterns' => Piwik\DI::factory(function (Container $c) {

if ($c->has('ini.log.exclude_patterns')) {

$xcl = [];

foreach (explode("|", $c->get('ini.log.exclude_patterns')) as $p) {

$xcl[] = trim($p);

}

return $xcl;

}

return null;

}),

michalkleiner · 2025-01-28T21:50:47Z

plugins/Monolog/config/config.php

+        if ($c->has('ini.log.enable_json')) {
+            return 'json';
+        }


Can we not pass this just via ini.log.string_message_format below?

@michalkleiner I considered that but I found adding a new option is probably easier for users to understand. Also, in the viewer code https://github.com/matomo-org/plugin-LogViewer/blob/5a7687b3591566aa5cdd1235241e06fa9891415b/Log/Parser/Piwik.php#L25 doesn't use the format so might be a dead feature anyway

michalkleiner · 2025-01-28T21:51:12Z

plugins/Monolog/config/config.php

+        if ($c->has('ini.log.enable_json')) {
+            return 'json';
+        }


Can we not pass it via ini.log.string_message_format_trace below?

same as above, looks like a dead feature

Joseph Beeson and others added 3 commits January 27, 2025 16:20

wip

55d1f1b

add support for enable_json = true, custom_function_file, exclude_pat…

882f41c

…terns

Merge branch '5.x-dev' into dev-json-logs

4ad7b33

patrickli requested review from mneudert, sgiehl, samjf, shafeek-innocraft and marcel-innocraft January 28, 2025 20:03

Joseph Beeson and others added 3 commits January 28, 2025 21:31

PHPCS format

19852de

PHPCS format

b93f56f

Merge branch '5.x-dev' into dev-json-logs

718d399

michalkleiner reviewed Jan 28, 2025

View reviewed changes

Joseph Beeson and others added 4 commits January 30, 2025 12:13

add type hints, FB discussion

a5201fa

Merge branch '5.x-dev' into dev-json-logs

e59eeda

phpcs

f572fd4

remove useless line

8f121ca

krezreb mentioned this pull request Jan 31, 2025

Dev json logs matomo-org/plugin-LogViewer#99

Open

11 tasks

yoda conditioning

70d884b

exception cleanup

66c096f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dev json logs #22994

Dev json logs #22994

krezreb commented Jan 28, 2025 •

edited

Loading

patrickli commented Jan 28, 2025

michalkleiner Jan 28, 2025

michalkleiner Jan 28, 2025

michalkleiner Jan 28, 2025

michalkleiner Jan 28, 2025

krezreb Jan 30, 2025

michalkleiner Jan 30, 2025

krezreb Jan 31, 2025

michalkleiner Jan 28, 2025

michalkleiner Jan 28, 2025

michalkleiner Jan 28, 2025

michalkleiner Jan 28, 2025

michalkleiner Jan 28, 2025

krezreb Jan 30, 2025

michalkleiner Jan 28, 2025

michalkleiner Jan 28, 2025

krezreb Jan 30, 2025

michalkleiner Jan 28, 2025

krezreb Jan 30, 2025

	public function __construct($logMessageFormat, $allowInlineLineBreaks = true, $excludePatterns = null, $customFunctionFile = null)
	public function __construct($logMessageFormat, $allowInlineLineBreaks = true, array $excludePatterns = null, string $customFunctionFile = null)

		@@ -43,6 +48,21 @@ public function format(array $record)

		$message = trim($record['message']);

		if (gettype($this->excludePatterns) === "array") {

	if (gettype($this->excludePatterns) === "array") {
	if (!empty($this->excludePatterns)) {

	if ($this->logMessageFormat == 'json') {
	if ('json' === $this->logMessageFormat) {

	if (gettype($this->customFunction) === "string") {
	if (is_callable($this->customFunction)) {

Dev json logs #22994

Are you sure you want to change the base?

Dev json logs #22994

Conversation

krezreb commented Jan 28, 2025 • edited Loading

Description:

Review

patrickli commented Jan 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krezreb commented Jan 28, 2025 •

edited

Loading