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

Stack traces are missing variadic and not declared arguments #1645

Open
xwillq opened this issue Nov 20, 2023 · 8 comments
Open

Stack traces are missing variadic and not declared arguments #1645

xwillq opened this issue Nov 20, 2023 · 8 comments

Comments

@xwillq
Copy link

xwillq commented Nov 20, 2023

How do you use Sentry?

Self-hosted / on-premises

SDK version

4.0.1

Steps to reproduce

  1. Create function with variadic parameters or without parameters. Ex.:
function variadic(...$params) {...}
function noParams() {...}
  1. Call this function with some arguments.
  2. Throw an exception inside that function.

Expected result

Sentry shows all passed arguments in stack trace.

Actual result

Sentry shows only first argument for variadic function and no arguments for function without parameters.
With this code:

function variadic(...$params)
{
    noParams($params);
}

function noParams()
{
    throw new \Exception();
}

Artisan::command('debug', function () {
    variadic('a', 1, [123]);
});

I get this stack trace in Sentry:
CleanShot 2023-11-20 at 18 51 49@2x

@xwillq
Copy link
Author

xwillq commented Nov 20, 2023

I believe the culprit is \Sentry\FrameBuilder::getFunctionArgumentValues. It only takes values passed for defined parameters based on their position, which means it completely ignores parameters that weren't defined or parameters that have multiple values.

@xwillq
Copy link
Author

xwillq commented Nov 20, 2023

Also, \Sentry\FrameBuilder::getFunctionArgumentValues ignores nulls, since it uses isset() to check if argument was passed to the function, and it doesn't use default values if nothing was passed to the function.
This might not be an issue depending on what's the purpose in sending parameters to sentry. If it's to show the complete picture, that it probably would be useful to send those. But if it is supposed to give just enough information to reconstruct the rest from the code, then sending arguments that was actually passed would be enough.

@cleptric
Copy link
Member

I haven't tested this yet, but how are variadic parameters reflected in a native stack trace?

@xwillq
Copy link
Author

xwillq commented Nov 20, 2023

All ways to pass parameters to function look the same way in PHP 7.4. Functions' code:

function regularParams(string $s, int $i, array $a)
{
    variadic($s, $i, $a);
}

function variadic(...$params)
{
    noParams(...$params);
}

function noParams()
{
    throw new \Exception();
}

Artisan::command('debug', function () {
    try {
        regularParams('a', 1, [123]);
    } catch (\Throwable $e) {
        var_dump(array_slice($e->getTrace(), 0, 3));
    }
});

var_dump output:

array(3) {
  [0] =>
  array(4) {
    'file' =>
    string(23) "/app/routes/console.php"
    'line' =>
    int(29)
    'function' =>
    string(8) "noParams"
    'args' =>
    array(3) {
      [0] =>
      string(1) "a"
      [1] =>
      int(1)
      [2] =>
      array(1) {
        ...
      }
    }
  }
  [1] =>
  array(4) {
    'file' =>
    string(23) "/app/routes/console.php"
    'line' =>
    int(24)
    'function' =>
    string(8) "variadic"
    'args' =>
    array(3) {
      [0] =>
      string(1) "a"
      [1] =>
      int(1)
      [2] =>
      array(1) {
        ...
      }
    }
  }
  [2] =>
  array(4) {
    'file' =>
    string(23) "/app/routes/console.php"
    'line' =>
    int(39)
    'function' =>
    string(13) "regularParams"
    'args' =>
    array(3) {
      [0] =>
      string(1) "a"
      [1] =>
      int(1)
      [2] =>
      array(1) {
        ...
      }
    }
  }
}

@cleptric
Copy link
Member

We use the args to add the variables to the Sentry stack trace. Not sure if we can improve this much.

@xwillq
Copy link
Author

xwillq commented Nov 20, 2023

args has all required info. I made quick and maybe dirty fix.

private function getFunctionArgumentValues(\ReflectionFunctionAbstract $reflectionFunction, array $backtraceFrameArgs): array
{
    $argumentValues = [];

    foreach ($reflectionFunction->getParameters() as $reflectionParameter) {
        if (!empty($backtraceFrameArgs)) {
            // Since getParameters() returns parameters in order they were
            // declared, we can just remove first argument from array to
            // get value for first parameter.
            $value = array_shift($backtraceFrameArgs);
        } else {
            // No more args, we can get out of the loop.
            // (or we can continue and handle default values here)
            break;
        }

        $argumentValues[$reflectionParameter->getName()] = $value;
    }

    $lastParameter = last($reflectionFunction->getParameters()) ?: null;
    if ($lastParameter !== null && $lastParameter->isVariadic()) {
        $lastParameterName = $lastParameter->getName();
        // Since variadic parameters available as arrays inside of functions,
        // we can wrap passed value into array so Sentry would represent it
        // the same way we would see it in code.
        $argumentValues[$lastParameterName] = [
            $argumentValues[$lastParameterName],
        ];
    }

    if (!empty($backtraceFrameArgs)) {
        if ($lastParameter !== null && $lastParameter->isVariadic()) {
            $lastParameterName = $lastParameter->getName();
            // If we still have args and last parameter is variadic,
            // merge its value with remaining args.
            $argumentValues[$lastParameterName] = array_merge(
                $argumentValues[$lastParameterName],
                $backtraceFrameArgs,
            );
        } else {
            // If last parameter is not variadic, it means we passed undefined
            // parameters into function. They are probably used with
            // func_get_args(), or this could be a bug. In any case, it
            // would be useful to see them in Sentry.
            $parameterPosition = count($argumentValues) + 1;
            foreach ($backtraceFrameArgs as $argument) {
                $argumentValues['param' . $parameterPosition] = $argument;
                $parameterPosition += 1;
            }
        }
    }

    return $argumentValues;
}

I'm not sure about performance and all the edge cases, but it solves the problem. Here is the before and after for code snipper from my last comment.

Before

Stack frame for variadic function contains only one variable, for function without arguments it doesn't contain anything.
CleanShot 2023-11-20 at 21 27 03@2x

After

Both stack frames contain all passed variables.
CleanShot 2023-11-20 at 21 29 12@2x

I can open a merge request if you think this solution is good enough or could be used as a start.

@cleptric
Copy link
Member

Oh, if you are okay with just having the values, sure, please open a PR (including tests 😬).
I thought you wanted the argument names as well.

@xwillq
Copy link
Author

xwillq commented Nov 20, 2023

The variadic parameter has a name, params, though it was a confusing one in this context 😅.
For function without parameters at all we cannot do much, so I took the same name FrameBuilder uses when it cannot create reflection object.
I think thats the best we could do for this two cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

2 participants