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.

Sign up for GitHub

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

Jump to bottom

fix(contrib-auto-laravel): fix log level type issue #306

Closed
wants to merge 4 commits into from
Closed

fix(contrib-auto-laravel): fix log level type issue #306

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c28ea77
fix(contrib-auto-laravel): fix log level is string but underlying log…
flc1125 Oct 16, 2024
7a48cd9
fix(contrib-auto-laravel): fix log level is string but underlying log…
flc1125 Oct 16, 2024
1760bef
chore(laravel): split `shouldRecordLog` method for easier override
flc1125 Oct 16, 2024
aed62ae
chore(laravel): split `shouldRecordLog` method for easier override
flc1125 Oct 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/Instrumentation/Laravel/src/Watchers/LogWatcher.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,9 @@ public function recordLog(MessageLogged $log): void
$underlyingLogger = $this->logger->getLogger();

/** @phan-suppress-next-line PhanUndeclaredMethod */
if (method_exists($underlyingLogger, 'isHandling') && !$underlyingLogger->isHandling($log->level)) {
if (method_exists($underlyingLogger, 'isHandling') && !$underlyingLogger->isHandling(
$underlyingLogger->toMonologLevel($log->level))
Copy link
Collaborator

Choose a reason for hiding this comment

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

this seems like it would now only work with monolog as the underlying logger, but there could be others, right? is toMonologLevel a part of any common interface between loggers?

Copy link
Author

Choose a reason for hiding this comment

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

@brettmc
I'm not quite sure how many of the third-party libraries in the industry that comply with \Psr\Log\LoggerInterface support the method method_exists($underlyingLogger, 'isHandling').

  • Suppose there are many. Then I wonder if adding method_exists($underlyingLogger, 'toMonologLevel') is too specific.
  • Suppose there are few, and it is more for supporting Monolog. Then I think it is acceptable to add method_exists($underlyingLogger, 'toMonologLevel').

Of course, there is another way, which is to add an adapter to solve this problem, but the extension might be a bit cumbersome.

So, I'd like to know your thoughts.

Copy link
Author

Choose a reason for hiding this comment

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

Also, there is another method, that is, I define an interface of ShouldHandle and bring it in when doing new LogWatcher($this->instrumentation). It's probably like this:

<?php

declare(strict_types=1);

use Illuminate\Log\Events\MessageLogged;
use Illuminate\Log\LogManager;

interface ShouldHandle
{
    public function shouldHandle(LogManager $logger, MessageLogged $log): bool;
}

Copy link
Author

Choose a reason for hiding this comment

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

@brettmc
Another processing method. Because method_exists($underlyingLogger, 'isHandling') it is too special. This method is not supported in \Psr\Log\LoggerInterface.

As an official extension package, for this special case, I suggest that users can consider overriding the recordLog method of LogWatcher.

And for this package, we only need to add a shoudRecordLog method.

For this, what we may have to do:

Of course, there are some other additions. That is, it can be retrieved more conveniently through some configurations. Currently, our register entry has some limitations. But this may take some time to continuously improve.

) {
return;
}

Expand Down