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

add 'cannot find the file...' only once #1619

Merged
merged 1 commit into from
Dec 10, 2018
Merged

add 'cannot find the file...' only once #1619

merged 1 commit into from
Dec 10, 2018

Conversation

guwirth
Copy link
Collaborator

@guwirth guwirth commented Dec 8, 2018

  • reduce the noise in LOG file
  • add 'cannot file the file...' only once per file in CxxIssuesReportSensor

This change is Reviewable

@guwirth guwirth added this to the 1.2.1 milestone Dec 8, 2018
@guwirth guwirth self-assigned this Dec 8, 2018
@guwirth guwirth requested a review from ivangalkin December 8, 2018 17:28
Copy link
Contributor

@ivangalkin ivangalkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @guwirth)


cxx-sensors/src/main/java/org/sonar/cxx/sensors/utils/CxxIssuesReportSensor.java, line 145 at r1 (raw file):

LOG.warn

I know, that the previous severity (DEBUG) doesn't match with the severity from below (WARN). This doesn't look right. However I disagree with making this trace to WARN. The reason is that for multi-module projects there will be too many warnings.

I've described this situation using the coverage import as example. Please see #1508 (comment) . Similar can happen when importing one single (large) e.g. cppcheck report for multiple (small) modules.

Not sure which solution would be correct. Ideally there should be some code like...

   if processing_one_of_many_modules(sensorContext):
       LOG.debug()
   else:
       # processing the single-module project
       LOG.warn()

... I don't know if it's possible to determine the condition processing_one_of_many_modules reliable.

@guwirth
Copy link
Collaborator Author

guwirth commented Dec 8, 2018

@ivangalkin this PR is an improvement to the current state. Current solution add warnings even if files are already in notFoundFiles. I moved the warning to getInputFileIfInProject. Maybe not perfect but much better.

@ivangalkin
Copy link
Contributor

@guwirth I agree, it's better. After some second look at the functions

  • getInputFileIfInProject() and
  • createNewIssueLocationFile()

I believe, that the first one will produce less noise. Some background.

  • function getInputFileIfInProject() is a service function, which might be called multiple times per project/module. It's used in all sensors implicitly (because CxxIssuesReportSensor is a base class) and in some of them explicitly:
    • CppcheckParserV2.java
    • CxxDrMemorySensor.java
    • CxxValgrindSensor.java

That means, that the same warning for some particular file path might appear multiple times per module/project. But it appears only once per sensor, because results of getInputFileIfInProject() are cached in notFoundFiles;

  • function createNewIssueLocationFile() has a similar problem. It might be also called from multiple sensors + each sensor might call this function multiple time. However the warning was not associated with the cache. So it's potentially more verbose.

So it's ok from my side.

Copy link
Contributor

@ivangalkin ivangalkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @guwirth)

- reduce the noise in LOG file
- add 'cannot file the file...' only once per file in CxxIssuesReportSensor
@guwirth guwirth changed the title add 'cannot file the file...' only once add 'cannot find the file...' only once Dec 10, 2018
@guwirth
Copy link
Collaborator Author

guwirth commented Dec 10, 2018

@ivangalkin thx. Let's test it and further improve it in next version...

@guwirth guwirth merged commit 3d7f4f9 into SonarOpenCommunity:master Dec 10, 2018
@guwirth guwirth mentioned this pull request Dec 21, 2018
@guwirth guwirth deleted the cannot-find-the-file-1 branch December 27, 2018 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants