#632 : Move stack trace formatting out of Span class by amber0612 · Pull Request #694 · open-telemetry/opentelemetry-php

amber0612 · 2022-06-01T13:17:51Z

closes #632

welcome · 2022-06-01T13:17:52Z

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.

linux-foundation-easycla · 2022-06-01T13:17:54Z

The committers listed above are authorized under a signed CLA.

✅ login: amber0612 / name: Amber (c3066fa, 99a4273, c8365d1)

brettmc · 2022-06-02T02:11:55Z

@amber0612 - can you please fix up linting errors with this PR? (make style will do it, and make all to confirm locally that everything is happy)

codecov · 2022-06-02T09:42:39Z

Codecov Report

Merging #694 (c8365d1) into main (85282b5) will decrease coverage by 2.25%.
The diff coverage is 2.63%.

@@             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

Flag	Coverage Δ
7.4	`83.82% <2.63%> (-2.22%)`	⬇️
8.0	`83.87% <2.63%> (-2.21%)`	⬇️
8.1	`83.87% <2.63%> (-2.21%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/SDK/Common/Util/TracingUtil.php	`0.00% <0.00%> (ø)`
src/SDK/Trace/Span.php	`93.69% <100.00%> (-0.23%)`	⬇️
src/Context/Context.php	`82.92% <0.00%> (ø)`
src/Contrib/Prometheus/PrometheusExporter.php	`93.75% <0.00%> (ø)`
src/Context/FiberNotSupportedContextStorage.php
src/Context/ScopeBound/ScopeBoundPromise.php	`96.29% <0.00%> (ø)`
src/Context/FiberBoundContextStorageScope.php	`0.00% <0.00%> (ø)`
src/Context/ScopeBound/ContextHolder.php	`100.00% <0.00%> (ø)`
src/API/Trace/Propagation/B3MultiPropagator.php	`100.00% <0.00%> (ø)`
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 85282b5...c8365d1. Read the comment docs.

brettmc · 2022-06-02T09:48:55Z

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.

amber0612 · 2022-06-03T07:17:07Z

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...

Nevay · 2022-06-03T09:49:00Z

I would write them as .phpt tests to have a clean stacktrace. There is an open issue with PHPUnit adding an additional frame; we can use run-tests.php instead (at least for now).

--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.)

tidal · 2022-06-03T10:48:08Z

+
+use Throwable;
+
+class TracingUtil


Could you please rename the class to OpenTelemetry\SDK\Common\Exception\StackTraceFormatter and the method to just format, please?

tidal · 2022-06-03T10:48:51Z

+         */
+    public static function formatStackTrace(Throwable $e, array &$seen = null): string
+    {
+        $starter = $seen ? 'Caused by: ' : '';


Could you make Caused by: a constant, please?

tidal · 2022-06-03T10:49:36Z

+    {
+        $starter = $seen ? 'Caused by: ' : '';
+        $result = [];
+        if (!$seen) {


Could you please check for a null value here, please?

@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

tidal · 2022-06-03T10:51:01Z

+        $file = $e->getFile();
+        $line = $e->getLine();
+        while (true) {
+            $current = "$file:$line";


Could you make this a sprintf as above, please?

tidal · 2022-06-03T10:54:19Z

+
+            $traceHasKey = array_key_exists(0, $trace);
+            $traceKeyHasClass = $traceHasKey && array_key_exists('class', $trace[0]);
+            $traceKeyHasFunction = $traceKeyHasClass && array_key_exists('function', $trace[0]);


Could you please male classand function class constants, please?

tidal · 2022-06-03T10:56:15Z

+                $line === null ? '' : ':',
+                $line ?? ''
+            );
+            $seen[] = "$file:$line";


Could you make this a sprintf call, please?

tidal · 2022-06-03T10:56:44Z

+                $line ?? ''
+            );
+            $seen[] = "$file:$line";
+            if (!count($trace)) {


Could you please check for a numeric value here, please?

tidal · 2022-06-03T10:57:29Z

+            if (!count($trace)) {
+                break;
+            }
+            $file = array_key_exists('file', $trace[0]) ? $trace[0]['file'] : 'Unknown Source';


Could you male Unknown Source a class constant, please?

tidal · 2022-06-03T10:58:12Z

+                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;


Could you make line a class constant, please?

tidal · 2022-06-03T10:59:06Z

+            array_shift($trace);
+        }
+        $result = implode("\n", $result);
+        if ($prev) {


Could you check for a type here, please?

tidal · 2022-06-03T13:13:48Z

@brettmc
We can take care of a test later,
so I'm merging this for now.

amber0612 added 2 commits June 1, 2022 16:26

Otel-php:632 Move stack trace formatting out of Span class

c3066fa

added function usages

99a4273

amber0612 requested review from bobstrecansky, brettmc, tidal and zsistla as code owners June 1, 2022 13:17

brettmc approved these changes Jun 1, 2022

View reviewed changes

fix linting errors

c8365d1

tidal suggested changes Jun 3, 2022

View reviewed changes

tidal approved these changes Jun 3, 2022

View reviewed changes

tidal merged commit e728c53 into open-telemetry:main Jun 3, 2022

Uh oh!

Conversation

amber0612 commented Jun 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

welcome Bot commented Jun 1, 2022

Uh oh!

linux-foundation-easycla Bot commented Jun 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brettmc commented Jun 2, 2022

Uh oh!

codecov Bot commented Jun 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

brettmc commented Jun 2, 2022

Uh oh!

amber0612 commented Jun 3, 2022

Uh oh!

Nevay commented Jun 3, 2022

Uh oh!

tidal Jun 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tidal commented Jun 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

amber0612 commented Jun 1, 2022 •

edited

Loading

linux-foundation-easycla Bot commented Jun 1, 2022 •

edited

Loading

codecov Bot commented Jun 2, 2022 •

edited

Loading

tidal Jun 3, 2022 •

edited

Loading