Add bug path length column to Bug overview #1209

csordasmarton · 2017-12-04T13:38:28Z

Closes #1160

Add bug path length column to Bug overview table to indicate how long the report path is.

Screenshot:

Xazax-hun · 2017-12-04T13:52:07Z

api/v6/report_server.thrift

@@ -156,7 +157,8 @@ struct ReportData {
  9: i64              column,          // column number of the report main section (not part of the path).
  10: Severity        severity,        // Checker severity.
  11: ReviewData      reviewData,      // Bug review status information.
-  12: DetectionStatus detectionStatus  // State of the bug (see the enum constant values).
+  12: DetectionStatus detectionStatus, // State of the bug (see the enum constant values).
+  13: i64             bugPathLength,   // Length of the bug path.


What is the policy on commas after the last item?

If we add a new item to this type, we don't have to modify the last item to place a comma at the end.

Xazax-hun · 2017-12-04T13:54:55Z

www/scripts/codecheckerviewer/ListOfBugs.js

+
+        reportData.bugPathLength = {
+          length : reportData.bugPathLength,
+          max : maxBugPathLength


Why do we need the max here?

We set a background color for each bug path length. We calculate this background color by using this maximum value (maximum length will be red). For more information see: generateRedGreenGradientColor in util.js.

But do we need this inside all report? Or no better place to put this? Also, this is the max on the displayed page or max in the run? In case the former, I wonder if that is the behavior the users are expecting.

whisperity · 2017-12-07T15:13:27Z

@csordasmarton Migration script is merged.

whisperity · 2017-12-07T15:29:02Z

www/scripts/codecheckerviewer/ListOfBugs.js

@@ -248,6 +270,7 @@ function (declare, dom, Deferred, ObjectStore, Store, QueryResults, topic,
        { name : 'Message', field : 'checkerMsg', width : '100%', formatter : checkerMessageFormatter },
        { name : 'Checker name', field : 'checkerId', width : '50%', formatter: checkerNameFormatter },
        { name : 'Severity', field : 'severity', cellClasses : 'severity', width : '15%', formatter : severityFormatter },
+        { name : 'Bug path length', field : 'bugPathLength', cellClasses : 'bug-path-length', width : '15%', formatter : bugPathLengthFormatter },


I think we should find a shorter name for this column. Isn't Length enough? When the header for the column is longer than the value printed, something's not quite right.

I think Difficulty is much better. What do you think @whisperity?

Sounds good to me.

whisperity · 2017-12-07T15:48:32Z

I think making the gradient happen from min to max is a problem, as min, max and average are very very sensitive to a single outlier.

Consider this example for bug path lenghts on three projects. That 42 length in Postgres makes that one the only really red one, and BPLs of 30-ish will be ever so slightly yellow, even though they are considerably less prominent in the code than short ones. Perhaps using an interquartile-range based approach is better?

dkrupp · 2017-12-08T16:23:54Z

Coloring should not depend on the actual result list. I suggest using a color scale that uniformly shows longer (impossible to understand) bug paths as reddish and more easily shorter ones as light greenish. You could for example say that you limit the bug path color range between [1,50] (being 50 and above impossible to understand) going gradually from light green to dark red.

bruntib · 2018-01-03T12:58:44Z

Is it really required to introduce a new column? Isn't it efficient enough to use the max(order) from the corresponding bug path table?

dkrupp

rename the column to Length or "Path Length" instead of difficulty. Otherwise fine.

dkrupp · 2018-01-03T13:24:42Z

www/scripts/codecheckerviewer/ListOfBugs.js

@@ -248,6 +265,7 @@ function (declare, dom, Deferred, ObjectStore, Store, QueryResults, topic,
        { name : 'Message', field : 'checkerMsg', width : '100%', formatter : checkerMessageFormatter },
        { name : 'Checker name', field : 'checkerId', width : '50%', formatter: checkerNameFormatter },
        { name : 'Severity', field : 'severity', cellClasses : 'severity', width : '15%', formatter : severityFormatter },
+        { name : 'Difficulty', field : 'bugPathLength', cellClasses : 'bug-path-length', width : '15%', formatter : bugPathLengthFormatter },


Name it simply to Length or Bug Path length as it correctly expresses what the number means. "Difficulty" is not expressive enough

dkrupp

Increase API version, since the thrift API has been updated

dkrupp · 2018-01-08T16:05:33Z

api/v6/report_server.thrift

@@ -68,7 +68,8 @@ enum SortType {
  CHECKER_NAME,
  SEVERITY,
  REVIEW_STATUS,
-  DETECTION_STATUS
+  DETECTION_STATUS,


API version needs to be incremented?

Xazax-hun

After Daniel's comments are resolved it LGTM!

whisperity · 2018-01-09T14:56:22Z

api/v6/report_server.thrift

@@ -68,7 +68,8 @@ enum SortType {
  CHECKER_NAME,
  SEVERITY,
  REVIEW_STATUS,
-  DETECTION_STATUS
+  DETECTION_STATUS,


Does the comment about "comma after last item" hold here? That would reduce the number of line changes in enums/structs too, assuming it's not a syntax error.

Add bug path length column to `Bug overview` table to indicate how long the report path is.

dkrupp

LGTM

csordasmarton added enhancement 🌟 API change 📄 Content of patch changes API! GUI 🎨 labels Dec 4, 2017

Xazax-hun reviewed Dec 4, 2017

View reviewed changes

whisperity added the RDY-OnHold 🛑 Patch reviewed and ready, but don't merge due to having to merge a dependent patch first. label Dec 5, 2017

whisperity removed the RDY-OnHold 🛑 Patch reviewed and ready, but don't merge due to having to merge a dependent patch first. label Dec 7, 2017

whisperity reviewed Dec 7, 2017

View reviewed changes

csordasmarton force-pushed the bug_path_length branch 2 times, most recently from 63087b5 to 52589bf Compare December 11, 2017 10:30

dkrupp requested changes Jan 3, 2018

View reviewed changes

dkrupp requested changes Jan 8, 2018

View reviewed changes

csordasmarton added the WIP 💣 Work In Progress label Jan 8, 2018

Xazax-hun approved these changes Jan 9, 2018

View reviewed changes

whisperity reviewed Jan 9, 2018

View reviewed changes

Add bug path length column to Bug overview

487f800

Add bug path length column to `Bug overview` table to indicate how long the report path is.

csordasmarton force-pushed the bug_path_length branch from 52589bf to 487f800 Compare January 9, 2018 16:04

csordasmarton requested review from dkrupp and bruntib January 9, 2018 16:06

dkrupp approved these changes Jan 10, 2018

View reviewed changes

dkrupp merged commit 2621a11 into Ericsson:master Jan 10, 2018

csordasmarton deleted the bug_path_length branch January 10, 2018 16:30

csordasmarton removed the WIP 💣 Work In Progress label Jan 12, 2018

csordasmarton mentioned this pull request Jan 15, 2018

Sorting results based on path length #362

Closed

whisperity mentioned this pull request Apr 9, 2018

Update BugStats.py to be able to use CodeChecker 6 invocation Xazax-hun/csa-testbench#18

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bug path length column to Bug overview #1209

Add bug path length column to Bug overview #1209

csordasmarton commented Dec 4, 2017 •

edited by whisperity

Loading

Xazax-hun Dec 4, 2017

csordasmarton Dec 4, 2017

Xazax-hun Dec 4, 2017

csordasmarton Dec 4, 2017

Xazax-hun Dec 4, 2017

whisperity commented Dec 7, 2017

whisperity Dec 7, 2017

csordasmarton Dec 11, 2017

whisperity Dec 11, 2017

whisperity commented Dec 7, 2017

dkrupp commented Dec 8, 2017

bruntib commented Jan 3, 2018

dkrupp left a comment

dkrupp Jan 3, 2018

dkrupp left a comment

dkrupp Jan 8, 2018

Xazax-hun left a comment

whisperity Jan 9, 2018

dkrupp left a comment

Add bug path length column to Bug overview #1209

Add bug path length column to Bug overview #1209

Conversation

csordasmarton commented Dec 4, 2017 • edited by whisperity Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whisperity commented Dec 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whisperity commented Dec 7, 2017

dkrupp commented Dec 8, 2017

bruntib commented Jan 3, 2018

dkrupp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkrupp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xazax-hun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkrupp left a comment

Choose a reason for hiding this comment

csordasmarton commented Dec 4, 2017 •

edited by whisperity

Loading