RestComm / gmlc

Restcomm Location Server
http://www.restcomm.com/
GNU Affero General Public License v3.0
17 stars 40 forks source link

Review logging level, and guarding/params #77

Closed jaimecasero closed 8 years ago

jaimecasero commented 8 years ago

https://github.com/RestComm/gmlc/blob/master/core/slee/services/sbbs/src/main/java/org/mobicents/gmlc/slee/MobileCoreNetworkInterfaceSbb.java#L443

As an example, the previous log should be debug, and guarded with level checking to prevent string format effort

jaimecasero commented 8 years ago

https://github.com/RestComm/gmlc/blob/master/core/slee/services/sbbs/src/main/java/org/mobicents/gmlc/slee/MobileCoreNetworkInterfaceSbb.java#L634

another case

FerUy commented 8 years ago

Noted, thanks @jaimecasero, will go through it today.

FerUy commented 8 years ago

@jaimecasero, just to be in sync, as we are using javax.slee.facilities.Tracer for the class private attribute "logger", trace levels available are from highest to lowest:

So, I believe that for the two cases you pointed out here, you refer to FINEST. If that's the case, your proposed solution would be for each examples like the following:

if (logger.isFinestEnabled()){ logger.finest(String.format("Handling %s request, msisdn: %s", httpRequestType.name().toUpperCase(), requestingMSISDN)); }

if (logger.isFinestEnabled()){ logger.finest("HTTP Request received and response sent, responseData=" + responseData); }

... right?

Or would you prefer FINE, like it's used for example in onDialogDelimiter public method? I personally think FINE is OK, but would like your opinion on this. Thanks in advance.

jaimecasero commented 8 years ago

those levels are borrowed form JDK logging https://docs.oracle.com/javase/7/docs/api/java/util/logging/Level.html

Usual usage is FINE==DEBUG, FINEST=TRACE.

For those examples I mentioned anything under CONFIG would be enough. You may use FINEST, since you are mostly tracing where the service logic started to work, not a computation of any actual logic.

The modification shows on your comment seems correct with proper guardgin (ifFinestEnabled..)

FerUy commented 8 years ago

Great, thanks @jaimecasero for your kind feedback. So, FINE will be fine ;)

FerUy commented 8 years ago

@jaimecasero I believe all important logging events are properly leveled and guarded whenever is due now. Please feel free to comment and reopen the issue if you feel something is missing or incorrectly set (and thanks again for the catch, notice and opening this one).