Refactored the logging #2010

Merged
merged 1 commit into from Dec 27, 2015

Projects

None yet

3 participants

@yueh
Member
yueh commented Nov 22, 2015

Using LogManager instead of FMLRelaunchLog to access the logger instance.
Added logging of the name of failed exports instead of exception.
Removed superfluous FMLRelaunchLog instance.
Removed superfluous parameters for PlayerData constructor.

Closes #2009
Refs #2069

@thatsIch thatsIch and 1 other commented on an outdated diff Nov 25, 2015
src/main/java/appeng/core/AELog.java
}
}
- public static void severe( final String format, final Object... data )
+ /**
+ * Use this to log caught {@link Exception} inside a try/catch block when necessary.
+ *
+ * Mostly intended for ignored exceptions.
+ *
+ * @param exception
+ */
+ public static void catching( @Nonnull final Throwable exception )
@thatsIch
thatsIch Nov 25, 2015 Member

I dont think the intention is clear enough. I would rather not log the exception before logging this for non purpose. Either its a warning, bug or additional information. I would not know how to fit in this usecase.
Because why are all errors suddenly moved to WARN? An error is clearly above a warning level.

The problem arises from the unclear uses of exceptions. Obviously the codebase is too complex to just remove these.

@yueh
yueh Nov 25, 2015 Member

When actually looking at the majority of the current AELog.error() uses, it is really just about log that exception for debug purposes. Rarely the catch block even tries to recover from it.
Thus debug is in these cases probably sufficient and error() should be called exception() since the beginning.

If we log any exception without any futher information for a reader outside a debug environment, I would say it is already a bad habit and should be reworked. There should be at least one other useful message similar to the change for #2009.
Best case would be in my opinion, log a meanigful message about what went wrong (should the user actually be able to read it) and have the stacktrace only as additional output in debug mode.
If it is a one time error and not reproduceable, we probably cannot even do anything with a stacktrace, otherwise we can simply verify it locally or ask the user to activate debug logging and post the verbose log.

@thatsIch thatsIch and 1 other commented on an outdated diff Nov 25, 2015
src/main/java/appeng/core/AELog.java
}
}
- public static void grinder( final String o )
+ /**
+ * Log an exception with a custom message formated via {@link String#format(String, Object...)}
+ *
+ * Similar to {@link AELog#log(Level, String, Object...)}.
+ *
+ * @see AELog#log(Level, String, Object...)
+ *
+ * @param exception
+ * @param message the message to be formatted.
+ * @param params the parameters used for {@link String#format(String, Object...)}.
+ */
+ public static void log( @Nonnull final Throwable exception, @Nonnull String message, final Object... params )
@thatsIch
thatsIch Nov 25, 2015 Member

if its warn why just called log. Missing intention in name of method. Logger.log should be a general purpose logging as System.out.print is with no intention at all.

@yueh
yueh Nov 25, 2015 Member

Currently I want to keep the amount of methods as small as possible. It is probably fine to change WARN to ERROR here and should the need arise for a different loglevel, then extend it accordingly.

I really do not want to let AELog implement Logger with like hundred different methods and proxy everything. Just because we can have a more fine grained logging, because it is not possible to do this via forge.

@yueh yueh added this to the rv3 - 1.7.10 milestone Nov 28, 2015
@yueh yueh Refactored the logging
Using LogManager instead of FMLRelaunchLog to access the logger instance.
Added logging of the name of failed exports instead of exception.
Improved crafting log to include issuer including their location and the
requested item.
Removed superfluous FMLRelaunchLog instance.
Removed superfluous parameters for PlayerData constructor.

Closes #2009
Refs #2069
c9ef1be
@orod-org orod-org commented on the diff Dec 26, 2015
src/main/java/appeng/core/AELog.java
{
- log( Level.DEBUG, "grinder: " + o );
+ final String formattedMessage = String.format( message, params );
+ final Logger logger = getLogger();
@orod-org
orod-org Dec 26, 2015

MINOR Make the "logger" logger private static final and rename it to comply with the format "LOG(?:GER)?". rule

@orod-org orod-org commented on the diff Dec 26, 2015
src/main/java/appeng/core/AELog.java
{
- FMLRelaunchLog.log( "AE2:" + ( Platform.isServer() ? "S" : "C" ), level, format, data );
+ final String formattedMessage = String.format( message, params );
+ final Logger logger = getLogger();
@orod-org
orod-org Dec 26, 2015

MINOR Make the "logger" logger private static final and rename it to comply with the format "LOG(?:GER)?". rule

@orod-org orod-org commented on the diff Dec 26, 2015
src/main/java/appeng/core/AELog.java
private AELog()
{
}
- public static void warning( final String format, final Object... data )
+ /**
+ * Returns a {@link Logger} logger suitable for the effective side (client/server).
+ *
+ * @return a suitable logger instance
+ */
+ private static Logger getLogger()
+ {
+ final String loggerName = LOGGER_PREFIX + ( Platform.isServer() ? SERVER_SUFFIX : CLIENT_SUFFIX );
+ final Logger logger = LogManager.getLogger( loggerName );
@orod-org
orod-org Dec 26, 2015

MINOR Immediately return this expression instead of assigning it to the temporary variable "logger". rule
MINOR Make the "logger" logger private static final and rename it to comply with the format "LOG(?:GER)?". rule

@orod-org

SonarQube analysis reported 4 issues:

  • MINOR 4 minor

Watch the comments in this conversation to review them.

@yueh yueh merged commit 17bdd08 into AppliedEnergistics:master Dec 27, 2015

3 checks passed

default Finished TeamCity Build Applied Energistics :: Pull Requests : Tests passed: 55
Details
jenkins Success 55 tests run, 0 skipped, 0 failed.
Details
sonarqube SonarQube reported 4 issues, no critical nor blocker
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment