monolog icon indicating copy to clipboard operation
monolog copied to clipboard

NormalizerFormatter skips some stacktraces

Open WoZ opened this issue 2 years ago • 5 comments

Monolog version 2|3

Check the function.

https://github.com/Seldaek/monolog/blob/5579edf28aee1190a798bfa5be8bc16c563bd524/src/Monolog/Formatter/NormalizerFormatter.php#L230-L235

PHP Fatal Error caught by register_shutdown_function() has trace field. Inside this trace field there can be few elements without file and line fields.

Here is an example:

# php test.php 
PHP Fatal error:  Allowed memory size of 134217728 bytes exhausted (tried to allocate 20480 bytes) in /var/www/html/test.php on line 27
array(1) {
  [0]=>
  array(1) {
    ["function"]=>
    string(16) "shutdownFunction"
  }
}

Run this script and you will see the following output of the $e->getTrace() method:

You may change register(false) to register(true) and check the results:

# php test.php 
PHP Fatal error:  Allowed memory size of 134217728 bytes exhausted (tried to allocate 20480 bytes) in /var/www/html/test.php on line 28
array(2) {
  [0]=>
  array(3) {
    ["file"]=>
    string(22) "/var/www/html/test.php"
    ["line"]=>
    int(11)
    ["function"]=>
    string(16) "shutdownFunction"
  }
  [1]=>
  array(3) {
    ["function"]=>
    string(14) "shutdownMethod"
    ["class"]=>
    string(12) "ErrorHandler"
    ["type"]=>
    string(2) "::"
  }
}

All that records without file and line will be skipped and engineer can't understand the full picture.

If you agree with this I may provide PR.

WoZ avatar Jul 19 '22 08:07 WoZ

I would rather not change the format, right now it normalizes to an array of file/line arrays. If you think you can fix it without changing the data type then please send a PR (or tell me how if you'd rather discuss first).

Seldaek avatar Jul 22 '22 13:07 Seldaek

@Seldaek I agree with you in general. I've found that sentry does the following with such errors.

#3 /app/main/Yii.php(62): Yii::error
#2 /app/vendor/yiisoft/yii2/base/ErrorHandler.php(318): yii\base\ErrorHandler::logException
#1 /app/vendor/yiisoft/yii2/base/ErrorHandler.php(283): yii\base\ErrorHandler::handleFatalError
#0 [internal](0): null

So, we may assume that someone parses an array for file/line string elements. The only option is to use a semicolon as a delimiter. So, the option is to mimic this format not to break compatibility.

Option 1. [internal]:0, as sentry does Option 2. internal[shutdownFunction]:0 and internal[ErrorHandler][shutdownMethod]:0 Option 3. internal(shutdownFunction):0 and internal[ErrorHandler](shutdownMethod):0

P.S. I missed to attach my test.php script.

<?php

function shutdownFunction() {
    $e = error_get_last();
    $exception = new \ErrorException($e['message'], $e['type'], $e['type'], $e['file'], $e['line']);
    var_dump($exception->getTrace());
}

class ErrorHandler {
    public static function shutdownMethod() {
        shutdownFunction();
    }
}

function register($classOrNot = false) {
    if ($classOrNot) {
        register_shutdown_function([ErrorHandler::class, 'shutdownMethod']);
    } else {
        register_shutdown_function('shutdownFunction');
    }
}

register(false);

$buffer = [];
while (true) {
    $buffer[] = str_repeat('+', 1024);
}

WoZ avatar Jul 23 '22 06:07 WoZ

Id maybe do this as option 4 for methods but otherwise yes I like this idea:

internal[ErrorHandler->shutdownMethod]:0

Seldaek avatar Jul 23 '22 06:07 Seldaek

For me ErrorHandler->methodName may be confusing if this is static method.

ErrorHandler.methodName is less confusing imho.

On Sat, Jul 23, 2022, 09:52 Jordi Boggiano @.***> wrote:

Id maybe do this as option 4 for methods but otherwise yes I like this idea:

internal[ErrorHandler->shutdownMethod]:0

— Reply to this email directly, view it on GitHub https://github.com/Seldaek/monolog/issues/1736#issuecomment-1193073593, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAU6HW4TRSVV4EP2E5GRMLVVOJBPANCNFSM537ASDPA . You are receiving this because you authored the thread.Message ID: @.***>

WoZ avatar Jul 23 '22 19:07 WoZ

Sure, the point was just to avoid : :)

Seldaek avatar Jul 23 '22 20:07 Seldaek