Skip to content

Comments

#6499 change log level to warning for log message issued in exception…#6500

Closed
pkiraly wants to merge 1 commit intoIQSS:developfrom
pkiraly:6499-change-log-level-4-exceptions
Closed

#6499 change log level to warning for log message issued in exception…#6500
pkiraly wants to merge 1 commit intoIQSS:developfrom
pkiraly:6499-change-log-level-4-exceptions

Conversation

@pkiraly
Copy link
Member

@pkiraly pkiraly commented Jan 10, 2020

… handling

What this PR does / why we need it: This pull request make errors and non error messages separated in the log.

Which issue(s) this PR closes: #6499 Change logging level from fine to warn for exceptions

Is there a release notes update needed for this change?: Improve logging

@coveralls
Copy link

Coverage Status

Coverage remained the same at 19.504% when pulling 6796bba on pkiraly:6499-change-log-level-4-exceptions into af4c0b0 on IQSS:develop.

@pdurbin
Copy link
Member

pdurbin commented Jan 10, 2020

@pkiraly I scrolled through the 20 files you changed and I think your change is fine (thought I don't know how "chatty" the logs will become).

Before we move this to QA, can you please provide a list of what to test?

@pkiraly
Copy link
Member Author

pkiraly commented Jan 10, 2020

@pdurbin The whole thing comes from the following scenario. A user get blank screen after creating a Dataverse. I investigated the case, and I found the following message in the Glassfish log:

[2020-01-10T11:12:43.130+0100] [glassfish 4.1] [FINE] [] [edu.harvard.iq.dataverse.util.BundleUtil] [tid: _ThreadID=52 _ThreadName=jk-connector$
  string found: Contact {0}]]

[2020-01-10T11:12:43.132+0100] [glassfish 4.1] [FINE] [] [edu.harvard.iq.dataverse.util.BundleUtil] [tid: _ThreadID=52 _ThreadName=jk-connector$
  string found: Support]]

[2020-01-10T11:13:01.144+0100] [glassfish 4.1] [FINE] [] [edu.harvard.iq.dataverse.DataverseServiceBean] [tid: _ThreadID=55 _ThreadName=jk-conn$
  Unable to find a single dataverse using alias "pk3": javax.persistence.NoResultException: getSingleResult() did not retrieve any entities.]]

Before that there are hundreds of log entries similar to the first two (edu.harvard.iq.dataverse.util.BundleUtil, string found: ....) entries, which are in my context is not an important information, and makes the important NoResultException almost invisible (like a needle in a haystack). This change makes the entry [WARNING], so I can filter it.

I haven't check the other exceptions, so I can not tell you exact use cases for testing them.

@pdurbin
Copy link
Member

pdurbin commented Jan 10, 2020

@pkiraly thanks, I'm going to move this to code review so I can get some opinions from others. I'm sort of wondering if you'd be willing to make a smaller pull request changing just the files that are affecting you directly. 2 files instead of 20 files or whatever. That way, it becomes clear exactly what to test.

Oh, also, if you want, you could read and edit our guidance on logging here: http://guides.dataverse.org/en/4.18.1/developers/coding-style.html#logging

Here's a screenshot:

Screen Shot 2020-01-10 at 9 36 10 AM

@pkiraly
Copy link
Member Author

pkiraly commented Jan 10, 2020

@pdurbin Thanks! I know it. The problem is clearly this: if you turn off fine level logging, and your exceptions throws fine log, you will not catch the exceptions. The exceptions I checked in this pull requests seems to be important enough to be on the radar.

I'll create another pull request with the minimal set.

@djbrooke
Copy link
Contributor

I'll close this PR since the smaller change was added in #6502. Thanks @pkiraly !

@djbrooke djbrooke closed this Jan 10, 2020
@pkiraly
Copy link
Member Author

pkiraly commented Jun 24, 2021

This PR was developed by Göttingen eResearch Alliance, Germany, and funded by SSHOC, "Social Sciences and Humanities Open Cloud". SSHOC has received funding from the European Union’s Horizon 2020 project call H2020-INFRAEOSC-04-2018, grant agreement #823782.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants