Add function complexity and size metrics #1398
Conversation
|
@ericlemes Hi, please just create another commit and sync it or close obsolete PRs. SonarQube server fails to start: |
|
Hi @Bertk, they are 3 independent pull requests. None are obsolete. I can merge them in one, if this facilitates the review process. |
|
@ericlemes Hi, |
|
@Bertk, you are right. It contains the other commits. I'll close the other PR's then. I still didn't understand why it is not starting sonar server. My local version works. I'll investigate further. |
|
The reason why it is failing is because I'm registering the metrics on both plugins (C and C++) and they have the same ID. I'm not sure which is the right way of doing this and will investigate further. If you load only one once, works fine. I'm not sure if the right way here is having one different metric ID per language (does not make much sense for me), or have one separated plugin just to register the metrics. |
|
@ericlemes before going into details:
|
|
Hi @guwirth The goals here are:
What someone can derive is:
Regards, Eric |
Here the question is if cyclomatic complexity or cognitive complexity is the right choice to us.
Somewhere I saw / read that you like to create files on server side. Think that's not a good idea because it's not ensured to have write access. Right way would be to create a REST API to ask for it? |
|
Hi @guwirth, I'd keep both complexities. The way the metric is expressed as %, can be done for cognitive complexity as well. In terms of the file dump. What I'm trying to do is just the "drill down" per function. Since sonar doesn't have a mechanism which it can be sent to the server (measure per function instead of per file), my implementation just dumps to a file (scanner side) if the file name is provided as a property. I'm investigating options of how to upload this. I've tested these metrics in my sonar instance, and I'm quite happy with the results. I'm thinking about implementing another one which is the same idea of expressing as a %, but expressing % of Lines of Code in Functions with more than 20 lines of code and % of lines of code in functions with more than cyclomatic complexity 10. The idea is just to observe what is the distortion in the %. Probably one of both would be enough, but I personally don't see harm in keeping both views. I really enjoyed how the % of functions with cyclomatic complexity is more representative than the avg cc/function. In some repos I've analysed, the avg cc/function has a variation of .5 CC/function between 2 repos. Using the % of complex functions, the same 2 repos has difference of 1.5% of code in complex functions. |
|
So guys, after a while trying to figure out what is going on here, I finally got to the point. A lot of learning about sonar plugin building going on. The simple explanation is: I've defined new metrics in both plugins. You can't do that. Metrics should have a unique key and be shared across plugins. There are 2 fixes for this, one that is hack, the other one which is sounds right, but requires more work. I'm happy to do the work, but I'd like to have an idea about how likely is for this PR being merged before I do a lot of work then throw everything on the bin. To cut to the chase, I feel that you are not comfortable with the new metrics at all. Fix 1 (the hack): Define 2 different metrics, with different keys. One for C the other one for C++. The result won't be consolidated. I don't like this. Because I believe these metrics can be consolidated between different languages. I'd like to hear your thoughts before moving forward, so I can manage my expectations. Regards Eric |
|
@ericlemes like the ideas of the new metrics. I'm not sure if the c plugin has a future. So maybe we combine it anyway to one plugin again. Think what should not be done is to write local files during scanner run. So most likely fix 1 is ok. |
|
@guwirth nice to hear that. I think the discussion about the new metrics in a separated, non cxx project would make sense anyway. I'm not sure if the future of the c plugin would be blocking this discussion. Regarding the files, I just got some hope about that. I was chatting in the sonar list, and apparently it is possible to push this data and display in a custom JS page. I'll put some effort on that, but I'm happy to take this off the plugin to simplify the merge. So, can you help me giving some sort of "steps" we can follow together to help this code and this PR be accepted? In my point of view they are:
Does that make sense? Cheers Eric |
|
Hey @guwirth, I did some changes, removing the file dumps and code from C Plugin. Now apparently some integration tests are breaking and I have no idea why, since I'm not dumping any new warnings. Can you help me? |
|
Hi, 2 scenarios of the BDD tests are failing The log file shows the exception and the log files are located here: Please see cppcheck_project_c_cpp.log Best regards |
|
Thanks @Bertk . After I learnt how to read the logs, I finally managed to fix all the problems. |
|
@ericlemes is it ready to review? |
|
@guwirth, yes it is. |
| * along with this program; if not, write to the Free Software Foundation, | ||
| * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. | ||
| */ | ||
| package org.sonar.cxx.sensors.functioncomplexity; |
There was a problem hiding this comment.
Please add package-info.java.
squid:S1228 - Packages should have a javadoc file 'package-info.java'
| public class FunctionScoreComparator implements Comparator<FunctionScore>{ | ||
|
|
||
| @Override | ||
| public int compare(FunctionScore o1, FunctionScore o2) { |
There was a problem hiding this comment.
Please add a unit test.
common-java:InsufficientLineCoverage - Lines should have sufficient coverage by tests
| */ | ||
| package org.sonar.cxx.sensors.functioncomplexity; | ||
|
|
||
| import java.io.ByteArrayOutputStream; |
There was a problem hiding this comment.
Please remove unused imports.
| */ | ||
| package org.sonar.cxx.sensors.functioncomplexity; | ||
|
|
||
| import com.sonar.sslr.api.AstNode; |
There was a problem hiding this comment.
Please remove unused imports.
|
|
||
| public CxxFunctionComplexitySquidSensor(CxxLanguage language){ | ||
| this.cyclomaticComplexityThreshold = language.getIntegerOption(FUNCTION_COMPLEXITY_THRESHOLD_KEY).orElse(10); | ||
| LOG.debug("Cyclomatic complexity threshold: " + this.cyclomaticComplexityThreshold); |
There was a problem hiding this comment.
Please add log guard
rule pmd:GuardLogStatement
| public static final Metric<Integer> BIG_FUNCTIONS = new Metric.Builder("big_functions", "Big Functions", Metric.ValueType.INT) | ||
| .setDescription("Number of functions with too many lines") | ||
| .setDirection(Metric.DIRECTION_WORST) | ||
| .setQualitative(false) |
There was a problem hiding this comment.
Please use Boolean.FALSE
rule fb-contrib:NAB_NEEDLESS_BOOLEAN_CONSTANT_CONVERSION
| public static final Metric<Integer> COMPLEX_FUNCTIONS = new Metric.Builder("complex_functions", "Complex Functions", Metric.ValueType.INT) | ||
| .setDescription("Number of functions with high cyclomatic complexity") | ||
| .setDirection(Metric.DIRECTION_WORST) | ||
| .setQualitative(false) |
There was a problem hiding this comment.
Please use Boolean.FALSE
rule fb-contrib:NAB_NEEDLESS_BOOLEAN_CONSTANT_CONVERSION
|
|
||
| public CxxFunctionComplexitySquidSensor(CxxLanguage language){ | ||
| this.cyclomaticComplexityThreshold = language.getIntegerOption(FUNCTION_COMPLEXITY_THRESHOLD_KEY).orElse(10); | ||
| LOG.debug("Cyclomatic complexity threshold: " + this.cyclomaticComplexityThreshold); |
There was a problem hiding this comment.
add if (LOG.isDebugEnabled())
|
|
||
| public CxxFunctionSizeSquidSensor(CxxLanguage language){ | ||
| this.sizeThreshold = language.getIntegerOption(FUNCTION_SIZE_THRESHOLD_KEY).orElse(20); | ||
| LOG.debug("Function size threshold: " + this.sizeThreshold); |
There was a problem hiding this comment.
add if (LOG.isDebugEnabled())
| } | ||
|
|
||
| protected void registerSquidSensors(){ | ||
| if (this.language.getKey() == "c++"){ |
There was a problem hiding this comment.
do not use "c++" use constant for plugin key instead
| @@ -40,7 +40,7 @@ | |||
| public CxxLanguage(String key, Configuration settings) { | |||
| super(key); | |||
| this.settings = settings; | |||
| this.MetricsCache = new HashMap<>(); | |||
| this.MetricsCache = new HashMap<>(); | |||
| @@ -89,7 +93,7 @@ | |||
| public static final String JSON_COMPILATION_DATABASE_KEY = LANG_PROP_PREFIX + "jsonCompilationDatabase"; | |||
| public static final String SCAN_ONLY_SPECIFIED_SOURCES_KEY = LANG_PROP_PREFIX + "scanOnlySpecifiedSources"; | |||
| public static final String CPD_IGNORE_LITERALS_KEY = LANG_PROP_PREFIX + "cpd.ignoreLiterals"; | |||
| public static final String CPD_IGNORE_IDENTIFIERS_KEY = LANG_PROP_PREFIX + "cpd.ignoreIdentifiers"; | |||
| public static final String CPD_IGNORE_IDENTIFIERS_KEY = LANG_PROP_PREFIX + "cpd.ignoreIdentifiers"; | |||
|
keys in settings file: Wouldn't it be better something like? |
|
@ericlemes how does the results look like in the UI? Can you add a screenshot please? |
|
@ericlemes question: think the metrics are very generic and could be used for all languages. Wouldn't it be better to add it to an own plugin? Like to avoid that we have it in one version and one version later it's removed again. |
|
@ericlemes solution I expected to solve this was:
Why did you select this approach? |
|
@ericlemes code looks good 👍 |
|
Thanks @Bertk and @guwirth for the review. There's a lot going on here, but I'm glad that apart from all the requests you like the code ;-) First question to @Bertk: Where did you get all this feedback about the rules? Is there any instance of Sonar that the CI is pushing information? I was struggling to get this, run the integration tests locally and the code coverage locally, and that's one of the reasons that I got loads of violations. If you can help me setting up a good environment, I can definitely be more productive. And Java is probably my 3rd language, so I know very little about the environment. Second question to @guwirth. I totally agree with you. I think the metrics are very generic and should be implemented in a different plugin. But, I'd really like your thoughts and guidance on that. First, if they are generic and could be implemented to other languages (which I'm intending to do), I guess they should be outside sonar-cxx project. I was really thinking about something like sonar-opencommunity-metrics in a new github project. I'm definitely help to implement like that but I'd like to agree on general design before spending a lot of time refactoring everything. Also, I was really struggling with maven/sonar stuff to create a new project. If you could create a skeleton on Git Hub which I could fork, that would definitely not only save me time, but help us to be more assertive. Generally speaking, I'm really happy that you guys are keen to accept this code. I personally doing a lot of experimentation with the metrics to understand how the numbers can be generic and meaningful at the same time. I'm planning to do all the reviews, but I'd really would like a definition about the new plugin structure before moving forward. Can you help me with this? |
|
Here is the screenshot. The numbers are from WebKit project.
|
- Adding reference to sonar-opencommunity-metrics - Changing all code to reference metrics defined in sonar-opencommunity-metrics, which now is responsible for the aggregated numbers.
This reverts commit 008fd89.
ericlemes
force-pushed
the
dump-cc-and-loc
branch
from
ac0bb4d
to
2f729f2
Compare
|
@ericlemes please tell us when we can do final review. |
There was a problem hiding this comment.
I am not familiar with that API, so my comments are related to the style only:
- Please remove the trailing whitespaces
- Please consider using
if (condition) { statement; }instead ofif (condition) statement;even for a single-line statements
Also I am not sure about the visualization of this metric. My experience with the similar complexity issues reminds me about the breakdown of a the total complexity score like
<primary location> Line X: Total complexity is 10
<secondary location 0> Line X': Nested if +2
<secondary location 1> Line X'': Nested for loop +2
<secondary location 2> Line X''': Nested if +2 etc
Will the warning have this level of details?
|
@ivangalkin, I've fixed your style comments. Related to the visualization of the metric, I think your comment does not apply. This metrics does not raise any issues in the code. Thy only present high level metrics, based on all the code base. |
|
@guwirth Ready for final review. |
There was a problem hiding this comment.
@ericlemes thx for applying my review comments. I believe that writing for() { statement; } instead of for () statement; would be better (less error-prone) too. Same for while. It's up to you if you want to fix it.
guwirth
changed the title
|
Hi @ericlemes, |
|
Hey @guwirth. You are welcome. |
Redesign of language-specific metrics
The following problems were identified and solved:
PROBLEM 1
* CxxLanguage::MetricsCache was not filled while
construction of CxxLanguage object but only in
CxxSquidSensor::CxxSquidSensor()
SOLUTION
* Introduce CxxMetricsFactory.
* It has no dependencies to sensors.
* It creates language-dependent metrics in the constructor
of CxxSquidSensor
PROBLEM 2
* During the test MetricsCache was almost always empty.
In order to prevent warnings (?) missing metrics' lookups
were just ignored
SOLUTON
* Introduce `protected abstract Optional<CxxMetricsFactory.Key> CxxReportSensor::getMetricKey();`
* By means of Optional<> semantic sensors show if metric is required or not
* CxxLanguage::getMetric() checks thows an exception if metric hasn't been found
PROBLEM 3
* CxxLanguage::getMetric(<required metric key>) expected the key to be typed as String
* In order to fill the MetricsCache the format {METRIC_KEY} was used
* Cache lookups were done however in the format {LANG_PROP_KEY}-{METRIC_KEY}
SOLUTION
* In order to make the lookup key type-safe and less error-prone I introduced the type
`CxxMetricsFactory.Key`
PROBLEM 4a
* The plugin-specific metrics for public API were missing on module level
All CoreMetrics (incls. former Public API) have automatic aggregation:
1. ComputeEngine implements some server-side task
2. This task iterates over the project hierarchy
`FILE -> DIRECTORY/MODULE -> ... -> ... -> PROJECT`
and performs a recursive sum for all metrics raised
on children component
3. This sum is stored as aggregated metric for the parent component
4. The number of hierarchy levels can be arbitrary
5. Starting with version 6.2 the public API metrics were
deprecated and Sonar-CXX introduces its own plugin-specific
public API metrics
6. These custom metrics are not aggregated automatically
anymore
PROBLEM 4b
* PR SonarOpenCommunity#1398 introduced 9 new complexity metrics
They supported the aggregation of the file-level metrics.
The aggregation was implemeted inside of the `AstVisitor`,
however the aggregation was done for the parent module only.
Ths solution worked for the simple SonarQube projects only.
Due to hierarchy depth == 2, aggregated results were stored
on the project level. Multi-module projects however might
have an arbitrary depth. So there was no proper aggregation
for such projects.
After public API metrics were deprecated as CoreMetrics
the automatic aggregation
It was not enough to set them on the file level. One had to aggregate these
measurements and set the result on the module level. Otherwise
they were not displayed in the SonarQube's Activity UI
SOLUTION
* Unify implementation of custom metrics for Public-API and complexity
* Implement 2 Compute Engine tasks (see `MeasureComputer`), which perform
the aggregation and computation of these metrics
* Computaton/aggregation works for both types of SonarQube project -
the simple and the multi-module ones
PROBLEM 5
* The metric `CoreMetrics.COGNITIVE_COMPLEXITY` was measured but not persisted
MISC
* Test has been added
* Minor refactoring and comments
Contains "Complex Functions" metric, which counts number of function that has cyclomatic complexity greater than sonar.cxx.funccomplexity.threshold (default 10). "% of Complex Functions" is the same metric expressed as a percent ((complex functions * 100)/total functions).
"Big functions", counts number of functions with more than sonar.cxx.funcsize.threshold (default 20). "% of Big Functions" is the same expressed as a percent.
This change is