#632 : Move stack trace formatting out of Span class #694
Conversation
amber0612
requested review from
bobstrecansky,
zsistla,
tidal and
brettmc
as code owners
|
Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one. |
|
@amber0612 - can you please fix up linting errors with this PR? ( |
Codecov Report
@@ Coverage Diff @@
## main #694 +/- ##
============================================
- Coverage 86.07% 83.82% -2.26%
- Complexity 1151 1233 +82
============================================
Files 128 138 +10
Lines 2794 2980 +186
============================================
+ Hits 2405 2498 +93
- Misses 389 482 +93
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Can you sort out signing a CLA please? (We can't accept your contribution without this) And also try to write a unit test for the newly-created class, since code coverage has dropped with this refactor. |
Hi Bret, Unit test for formatStackTrace was not written previously. Can u help me with what am i supposed to assert equal for Unit test of the formatStackTrace function. As the function returns String which is stack Trace and string returned will be complex with Line numbers too. Do u have ideas on how to handle unit test for this... |
|
I would write them as --TEST--
Basic stacktrace format
--FILE--
<?php
use OpenTelemetry\SDK\Common\Util\TracingUtil;
require_once 'vendor/autoload.php';
echo TracingUtil::formatStackTrace(new Exception('message')), "\n";
?>
--EXPECT--
Exception: message
at main(test_stacktrace_basic.php:6)(I plan on creating a follow-up PR to resolve some issues with the current implementation and could take care of the tests if necessary.) |
|
|
||
| use Throwable; | ||
|
|
||
| class TracingUtil |
There was a problem hiding this comment.
Could you please rename the class to OpenTelemetry\SDK\Common\Exception\StackTraceFormatter and the method to just format, please?
| */ | ||
| public static function formatStackTrace(Throwable $e, array &$seen = null): string | ||
| { | ||
| $starter = $seen ? 'Caused by: ' : ''; |
There was a problem hiding this comment.
Could you make Caused by: a constant, please?
| { | ||
| $starter = $seen ? 'Caused by: ' : ''; | ||
| $result = []; | ||
| if (!$seen) { |
There was a problem hiding this comment.
Could you please check for a null value here, please?
There was a problem hiding this comment.
@tidal I plan on replacing the implementation with a reduced version of this after this PR is merged, which would make most of these changes obsolete; can we postpone implementation specific changes and keep this PR about moving the implementation out of the Span class?
See below for example output that highlights some problems, changes:
- Fix "... n more" to fold only identical frames, currently we might lose information (folding on first seen frame instead of folding last n identical frames as described by Java)
- Fix exception class names not being converted to dotted format
- Fix functions being shown as
main - Fix out-of-memory on circular exception
# new
Abc.Def.Test: bar
at Abc.Def.Test.create(Test.php:10)
at Abc.Def.run(example.php:8)
at {main}(example.php:12)
Caused by: Abc.Def.Test: foo
at Abc.Def.Test.create(Test.php:10)
at Abc.Def.run(example.php:7)
... 1 more
# current
Abc\Def\Test: bar
at Abc.Def.Test.create(Test.php:10)
at main(example.php:8)
at main(example.php:12)
Caused by: Abc\Def\Test: foo
... 3 more
| $file = $e->getFile(); | ||
| $line = $e->getLine(); | ||
| while (true) { | ||
| $current = "$file:$line"; |
There was a problem hiding this comment.
Could you make this a sprintf as above, please?
|
|
||
| $traceHasKey = array_key_exists(0, $trace); | ||
| $traceKeyHasClass = $traceHasKey && array_key_exists('class', $trace[0]); | ||
| $traceKeyHasFunction = $traceKeyHasClass && array_key_exists('function', $trace[0]); |
There was a problem hiding this comment.
Could you please male classand function class constants, please?
| $line === null ? '' : ':', | ||
| $line ?? '' | ||
| ); | ||
| $seen[] = "$file:$line"; |
There was a problem hiding this comment.
Could you make this a sprintf call, please?
| $line ?? '' | ||
| ); | ||
| $seen[] = "$file:$line"; | ||
| if (!count($trace)) { |
There was a problem hiding this comment.
Could you please check for a numeric value here, please?
| if (!count($trace)) { | ||
| break; | ||
| } | ||
| $file = array_key_exists('file', $trace[0]) ? $trace[0]['file'] : 'Unknown Source'; |
There was a problem hiding this comment.
Could you male Unknown Source a class constant, please?
| break; | ||
| } | ||
| $file = array_key_exists('file', $trace[0]) ? $trace[0]['file'] : 'Unknown Source'; | ||
| $line = array_key_exists('file', $trace[0]) && array_key_exists('line', $trace[0]) && $trace[0]['line'] ? $trace[0]['line'] : null; |
There was a problem hiding this comment.
Could you make line a class constant, please?
| array_shift($trace); | ||
| } | ||
| $result = implode("\n", $result); | ||
| if ($prev) { |
There was a problem hiding this comment.
Could you check for a type here, please?
|
@brettmc |
closes #632