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

Dev json logs #99

Open
wants to merge 4 commits into
base: 5.x-dev
Choose a base branch
from
Open

Dev json logs #99

wants to merge 4 commits into from

Conversation

krezreb
Copy link

@krezreb krezreb commented Jan 28, 2025

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

Related PR matomo-org/matomo#22994

@krezreb krezreb mentioned this pull request Jan 30, 2025
11 tasks

if (is_object($message)) {
$message->severity = $message->level;
return $message;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that there's no certainty of the encoded value in $line->content, it might be worth adding some checks or improved handling here. Since it's public method it should really return a class/interface for the message, but I appreciate it's not something you added so not something for today.
At a minimum I would null coalesce this level property reference, but i'd recommend populating all of the variables off the object and then letting the rest of this method run (if you want a consistent result). Is it intended that you return an object here when it normally returns an array?

Copy link
Author

@krezreb krezreb Jan 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcel-innocraft actually the field will be present because it's set in the proposed json encoding function. I had to add this line because in the writer code the field is referred to as level but in the logviewer it's severity. I'm under the assumption that only lines generated using that class will be parsed here

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assumptions are like the dangerous icebergs of code. When someone else starts logging their {"message": "Floating donkey"} messages then you'll run into trouble.
Having a class/interface would define the contract for working with these messages so then there are no assumptions being relied on, but short of increasing the scope with a refactor, I'd just make this a little safer. That's my 5c worth.

/*
* First try to json decode the message in case json logging is enabled
*/
$message = json_decode($line->content);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krezreb Can we read the config isJson logging enabled here ? and take an action accordingly, I assume if JSON logging is disabled but someone sends a logline as a JSON this will treat it as a JSON value.

Copy link
Author

@krezreb krezreb Jan 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AltamashShaikh that's a valid point. However I do not think that is a problem. If a user sends json content to the logger's message field it will still be prefixed when it appears in the logs, so it will fail the json_decode test.

Also in cases where there are historical non json encoded lines we need to keep the legacy reading. i'm not assuming users will think to purge their logs when they turn json_encodingn on

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants