Bug 1169757 - Serious logs without id/i18n support (RestEasy)
Summary: Serious logs without id/i18n support (RestEasy)
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: JBoss Enterprise Application Platform 6
Classification: JBoss
Component: Logging, RESTEasy
Version: 6.4.0
Hardware: Unspecified
OS: Unspecified
unspecified
urgent
Target Milestone: ER1
: EAP 6.4.0
Assignee: Ron Sigal
QA Contact: Petr Kremensky
URL:
Whiteboard:
Depends On:
Blocks: 1174117
TreeView+ depends on / blocked
 
Reported: 2014-12-02 11:33 UTC by Petr Kremensky
Modified: 2019-08-19 12:46 UTC (History)
3 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2019-08-19 12:46:04 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)
Test results (11.29 KB, text/plain)
2014-12-02 11:33 UTC, Petr Kremensky
no flags Details


Links
System ID Private Priority Status Summary Last Updated
Red Hat Issue Tracker EAP6-49 0 Major Verified Logging Unification for RESTEasy 2018-09-14 16:25:51 UTC

Description Petr Kremensky 2014-12-02 11:33:14 UTC
Created attachment 963653 [details]
Test results

Description of problem:
According to ANDIAMO-7 all error and warning messages should have a unique code. There are some serious log messages in RestEasy sources which doesn't fulfil this requirement. 

Version-Release number of selected component (if applicable):
EAP 6.4.0.DR11 - RestEasy 2.3.9

Additional info:
https://jenkins.mw.lab.eng.bos.redhat.com/hudson/job/eap-6x-manu-logging-unification-resteasy/1/testReport/junit/manu.testsuite.eap.eap6.logging/RestEasyLoggingTest/unsupportedLocalization__/

Adding blocker flag since this is related to https://issues.jboss.org/browse/EAP6-49

Comment 1 Petr Kremensky 2014-12-02 11:58:53 UTC
I am also quite curious why you use message loggers like [1]. You could follow the style from [2] and than use:
LogMessages.LOGGER.foundBeanManagerInServletContext();
instead of 
LogMessages.LOGGER.debug(Messages.MESSAGES.foundBeanManagerInServletContext());

However this isn't a functional problem.

[1] - https://github.com/resteasy/Resteasy/blob/Branch_2_3/resteasy-cdi/src/main/java/org/jboss/resteasy/cdi/i18n/LogMessages.java
[2] - https://github.com/resteasy/Resteasy/blob/Branch_2_3/resteasy-jaxrs/src/main/java/org/jboss/resteasy/resteasy_jaxrs/i18n/LogMessages.java

Comment 2 Ron Sigal 2014-12-03 04:17:50 UTC
Hi Petr,

Ok, here's what I see:

 * resteasy-spring-jetty: My fault. Don't know how I missed that.

 * resteasy-test-tjws: It's just a test module.

 * eagleDNS: The module name in pom.xml is "RESTEasy EagleDNS Fork For Testing Purposes"

 * resteasy-jaxrs:
   * Some of the references are to the org.jboss.resteasy.logging.impl, the original Resteasy logging layer. I stripped out all uses of it when I did the i18n changes.
   * ResteasyDeployment: My fault.

 * resteasy-spring: The test got fooled. The flagged line and the preceding line are

  String message = Messages.MESSAGES.pathNotInitialRequest(httpRequest.getUri().getPath());
  LogMessages.LOGGER.error(message);

I'll fix the problems in resteasy-spring-jetty and resteasy-jaxrs. I suppose it's way too late to get another release into EAP 6.4.0, but the fixes should go into the next Resteasy release.

Comment 3 Ron Sigal 2014-12-03 04:22:19 UTC
I created RESTEASY-1136 "Convert project to use i18n logging and exceptions - Part II" to track the changes I have to make.

Comment 4 Ron Sigal 2014-12-03 04:35:09 UTC
As for "I am also quite curious why you use message loggers like [1]", well, mostly, I just did. The resteasy-jaxrs module has by far the most messages, so I put the log messages in a separate file. For the other modules, it just seemed simpler to put all the messages in a single file. 

One advantage is that the logging level of the message is clear from the call, without referencing the messages file.

But, really, I don't have a compelling argument to make.

Comment 5 Ron Sigal 2014-12-03 04:37:28 UTC
Hey, Petr, at some point I'm going to make the same i18n changes to the Resteasy master branch. Is there a way I can use the jenkins tests to check up on myself?

Comment 6 Petr Kremensky 2014-12-03 13:26:22 UTC
Hi Ron,
thanks for checking the results. I'll update the tests to reflect your comment.

I am using @LogMessage level in localization tests to filter out only Warn/Error/Fatal messages to check that they are localized (however there is no requirement for localization on RestEasy so far and I should be able to update tests logic once required) and this is also listed among general rules in https://developer.jboss.org/wiki/InternationalizedLoggingAndExceptions, however I am not sure how strict are these.

Sure, I'll mail you link to get the tests with instructions how to run them against any sources you like locally.

Comment 7 Petr Kremensky 2014-12-05 13:29:55 UTC
I've updated tests to reflect comment 2

https://jenkins.mw.lab.eng.bos.redhat.com/hudson/job/eap-6x-manu-logging-unification-resteasy/2/testReport/manu.testsuite.eap.eap6.logging/RestEasyLoggingTest/unsupportedLocalization__/

I didn't exclude the functionality which is fooling the test in case of resteasy-spring message, as main goal of test is to locate suspicious logs and this functionality can find some more suspicious logs in future.

So please ignore:
ResteasyHandlerMapping.java - 69   LogMessages.LOGGER.error(message);

Comment 8 Petr Kremensky 2014-12-08 08:58:12 UTC
Ron, can you please make a new release with fixed messages and put it into 6.4.0, it's not too late yet. This one is RFE related and should be fixed in 6.4.0.

Comment 11 Petr Kremensky 2015-01-15 11:54:17 UTC
Verified on EAP 6.4.0.ER1.

Resteasy 2.3.10.Final-redhat


Note You need to log in before you can comment on or make changes to this bug.