From d09a4016d3cd8f3ea36534579d1a01b408715a20 Mon Sep 17 00:00:00 2001 From: Risto Lahtela <24460436+Rsl1122@users.noreply.github.com> Date: Thu, 14 May 2020 12:18:12 +0300 Subject: [PATCH] Rewrote error logging code - The new error logging only creates one log per error and has context with the error if specified. - Any duplicate lines in error stacktrace are not taken into account when hashing to avoid recursive function errors being logged in different files Affects issues: - Close #1246 --- .../plan/delivery/webserver/WebServer.java | 7 +- .../djrapitops/plan/modules/APFModule.java | 5 +- .../plan/storage/file/PlanFiles.java | 6 +- .../djrapitops/plan/utilities/java/Lists.java | 31 +++ .../plan/utilities/logging/ErrorContext.java | 77 ++++++ .../plan/utilities/logging/ErrorLogger.java | 242 ++++++++++++++++++ .../resources/assets/plan/bungeeconfig.yml | 2 +- .../src/main/resources/assets/plan/config.yml | 2 +- 8 files changed, 361 insertions(+), 11 deletions(-) create mode 100644 Plan/common/src/main/java/com/djrapitops/plan/utilities/logging/ErrorContext.java create mode 100644 Plan/common/src/main/java/com/djrapitops/plan/utilities/logging/ErrorLogger.java diff --git a/Plan/common/src/main/java/com/djrapitops/plan/delivery/webserver/WebServer.java b/Plan/common/src/main/java/com/djrapitops/plan/delivery/webserver/WebServer.java index 4798d8751..060abdcf2 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/delivery/webserver/WebServer.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/delivery/webserver/WebServer.java @@ -17,8 +17,6 @@ package com.djrapitops.plan.delivery.webserver; import com.djrapitops.plan.SubSystem; -import com.djrapitops.plan.identification.ServerInfo; -import com.djrapitops.plan.identification.properties.ServerProperties; import com.djrapitops.plan.settings.config.PlanConfig; import com.djrapitops.plan.settings.config.paths.PluginSettings; import com.djrapitops.plan.settings.config.paths.WebserverSettings; @@ -57,8 +55,7 @@ public class WebServer implements SubSystem { private final PlanFiles files; private final PlanConfig config; - private final ServerProperties serverProperties; - private Addresses addresses; + private final Addresses addresses; private final RequestHandler requestHandler; private final PluginLogger logger; @@ -75,7 +72,6 @@ public class WebServer implements SubSystem { Locale locale, PlanFiles files, PlanConfig config, - ServerInfo serverInfo, Addresses addresses, PluginLogger logger, ErrorHandler errorHandler, @@ -84,7 +80,6 @@ public class WebServer implements SubSystem { this.locale = locale; this.files = files; this.config = config; - this.serverProperties = serverInfo.getServerProperties(); this.addresses = addresses; this.requestHandler = requestHandler; diff --git a/Plan/common/src/main/java/com/djrapitops/plan/modules/APFModule.java b/Plan/common/src/main/java/com/djrapitops/plan/modules/APFModule.java index 96ece2542..1960d9e95 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/modules/APFModule.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/modules/APFModule.java @@ -17,6 +17,7 @@ package com.djrapitops.plan.modules; import com.djrapitops.plan.PlanPlugin; +import com.djrapitops.plan.utilities.logging.ErrorLogger; import com.djrapitops.plugin.IPlugin; import com.djrapitops.plugin.benchmarking.Timings; import com.djrapitops.plugin.command.ColorScheme; @@ -70,8 +71,8 @@ public class APFModule { @Provides @Singleton - ErrorHandler provideErrorHandler(IPlugin plugin) { - return plugin.getErrorHandler(); + ErrorHandler provideErrorHandler(ErrorLogger errorLogger) { + return errorLogger; } @Provides diff --git a/Plan/common/src/main/java/com/djrapitops/plan/storage/file/PlanFiles.java b/Plan/common/src/main/java/com/djrapitops/plan/storage/file/PlanFiles.java index 906cf934b..0313b6028 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/storage/file/PlanFiles.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/storage/file/PlanFiles.java @@ -59,7 +59,7 @@ public class PlanFiles implements SubSystem { } public Path getCustomizationDirectory() { - return dataFolder.toPath().resolve("web"); + return getDataDirectory().resolve("web"); } public File getLogsFolder() { @@ -72,6 +72,10 @@ public class PlanFiles implements SubSystem { } } + public Path getLogsDirectory() { + return getDataDirectory().resolve("logs"); + } + public File getConfigFile() { return configFile; } diff --git a/Plan/common/src/main/java/com/djrapitops/plan/utilities/java/Lists.java b/Plan/common/src/main/java/com/djrapitops/plan/utilities/java/Lists.java index 3d231fd46..060095594 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/utilities/java/Lists.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/utilities/java/Lists.java @@ -19,6 +19,7 @@ package com.djrapitops.plan.utilities.java; import java.util.*; import java.util.function.Function; import java.util.function.Predicate; +import java.util.function.UnaryOperator; /** * Methods that can be used as functional interfaces when dealing with Maps. @@ -35,6 +36,10 @@ public class Lists { return new ArrayList<>(); } + public static Lists.Builder builder(Class ofType) { + return new Lists.Builder<>(); + } + /** * Efficient replacement for List#stream().filter(keep).collect(Collectors.toList()). * @@ -85,4 +90,30 @@ public class Lists { } return mapped; } + + public static class Builder { + private final List list; + + private Builder() { + list = new ArrayList<>(); + } + + public Lists.Builder add(V value) { + list.add(value); + return this; + } + + public Lists.Builder addAll(Collection values) { + list.addAll(values); + return this; + } + + public Lists.Builder apply(UnaryOperator> operator) { + return operator.apply(this); + } + + public List build() { + return list; + } + } } \ No newline at end of file diff --git a/Plan/common/src/main/java/com/djrapitops/plan/utilities/logging/ErrorContext.java b/Plan/common/src/main/java/com/djrapitops/plan/utilities/logging/ErrorContext.java new file mode 100644 index 000000000..ee65e2e42 --- /dev/null +++ b/Plan/common/src/main/java/com/djrapitops/plan/utilities/logging/ErrorContext.java @@ -0,0 +1,77 @@ +/* + * This file is part of Player Analytics (Plan). + * + * Plan is free software: you can redistribute it and/or modify + * it under the terms of the GNU Lesser General Public License v3 as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * Plan is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with Plan. If not, see . + */ +package com.djrapitops.plan.utilities.logging; + +import java.util.*; + +/** + * Contains context for an error that might help debugging it. + * + * @author Rsl1122 + */ +public class ErrorContext { + + private final List related; + private String whatToDo; + + private ErrorContext() { + related = new ArrayList<>(); + } + + public static ErrorContext.Builder builder() { + return new ErrorContext.Builder(); + } + + public Optional getWhatToDo() { + return Optional.ofNullable(whatToDo); + } + + public Collection toLines() { + List lines = new ArrayList<>(); + for (Object o : related) { + lines.add(Objects.toString(o)); + } + return lines; + } + + public static class Builder { + private final ErrorContext context; + + public Builder() { + context = new ErrorContext(); + } + + public Builder whatToDo(String whatToDo) { + context.whatToDo = whatToDo; + return this; + } + + public Builder related(Object related) { + context.related.add(related); + return this; + } + + public Builder related(Object... related) { + context.related.addAll(Arrays.asList(related)); + return this; + } + + public ErrorContext build() { + return context; + } + } +} diff --git a/Plan/common/src/main/java/com/djrapitops/plan/utilities/logging/ErrorLogger.java b/Plan/common/src/main/java/com/djrapitops/plan/utilities/logging/ErrorLogger.java new file mode 100644 index 000000000..4e3c94675 --- /dev/null +++ b/Plan/common/src/main/java/com/djrapitops/plan/utilities/logging/ErrorLogger.java @@ -0,0 +1,242 @@ +/* + * This file is part of Player Analytics (Plan). + * + * Plan is free software: you can redistribute it and/or modify + * it under the terms of the GNU Lesser General Public License v3 as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * Plan is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with Plan. If not, see . + */ +package com.djrapitops.plan.utilities.logging; + +import com.djrapitops.plan.PlanPlugin; +import com.djrapitops.plan.delivery.formatting.Formatters; +import com.djrapitops.plan.identification.properties.ServerProperties; +import com.djrapitops.plan.storage.file.PlanFiles; +import com.djrapitops.plan.utilities.java.Lists; +import com.djrapitops.plan.version.VersionChecker; +import com.djrapitops.plugin.logging.L; +import com.djrapitops.plugin.logging.console.PluginLogger; +import com.djrapitops.plugin.logging.error.ErrorHandler; +import dagger.Lazy; +import org.apache.commons.codec.digest.DigestUtils; +import org.apache.commons.lang3.StringUtils; + +import javax.inject.Inject; +import javax.inject.Singleton; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.StandardOpenOption; +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import java.util.logging.Level; +import java.util.logging.Logger; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +/** + * New logger that logs errors to specific files. + * + * @author Rsl1122 + */ +@Singleton +public class ErrorLogger implements ErrorHandler { + + private final PlanPlugin plugin; + private final PluginLogger logger; + private final PlanFiles files; + private final Lazy serverProperties; + private final Lazy versionChecker; + private final Lazy formatters; + + @Inject + public ErrorLogger( + PlanPlugin plugin, + PluginLogger logger, + PlanFiles files, + Lazy serverProperties, + Lazy versionChecker, + Lazy formatters + ) { + this.plugin = plugin; + this.logger = logger; + this.files = files; + this.serverProperties = serverProperties; + this.versionChecker = versionChecker; + this.formatters = formatters; + } + + public void log(L level, Throwable throwable, ErrorContext context) { + String errorName = throwable.getClass().getSimpleName(); + String hash = hash(throwable); + Path logsDir = files.getLogsDirectory(); + Path errorLog = logsDir.resolve(errorName + "-" + hash + ".txt"); + if (Files.exists(errorLog)) { + logExisting(errorLog, throwable, context); + } else { + logNew(errorLog, throwable, context); + } + logToConsole(level, errorLog, throwable, context); + if (L.CRITICAL == level) { + plugin.getPluginLogger().error("CRITICAL error triggered a plugin shutdown."); + plugin.onDisable(); + } + } + + private void logExisting(Path errorLog, Throwable throwable, ErrorContext context) { + // Read existing + List lines; + try (Stream read = Files.lines(errorLog)) { + lines = read.collect(Collectors.toList()); + } catch (IOException e) { + logAfterReadError(errorLog, throwable, context); + return; + } + int occurrences = getOccurrences(lines) + 1; + List newLines = buildNewLines(context, lines, occurrences); + overwrite(errorLog, throwable, newLines); + } + + private void overwrite(Path errorLog, Throwable throwable, List newLines) { + try { + Files.write(errorLog, newLines, StandardOpenOption.TRUNCATE_EXISTING); + } catch (IOException e) { + throwable.addSuppressed(e); + Logger.getGlobal().log(Level.SEVERE, "Failed to log Plan error, see suppressed.", throwable); + } + } + + private List buildNewLines(ErrorContext context, List lines, int occurrences) { + Lists.Builder builder = Lists.builder(String.class) + .add("Last occurred: " + getTimeStamp() + " Occurrences: " + occurrences); + // 5 contexts are enough. + if (occurrences <= 5) { + builder = buildContext(context, occurrences, builder); + } + int lineCount = lines.size(); + int firstContextLineIndex = findFirstContextLine(lines, lineCount); + return builder.addAll(lines.subList(firstContextLineIndex, lineCount)) + .build(); + } + + private String getTimeStamp() { + return formatters.get().iso8601NoClockLong().apply(System.currentTimeMillis()); + } + + private Lists.Builder buildContext(ErrorContext context, int occurrences, Lists.Builder builder) { + return builder.add("---- Context " + occurrences + " ----") + .add("Plan v" + versionChecker.get().getCurrentVersion()) + .add(serverProperties.get().getName() + " " + serverProperties.get().getVersion()) + .add("Server v" + serverProperties.get().getImplVersion()) + .add("") + .addAll(context.toLines()) + .add(""); + } + + private void logAfterReadError(Path errorLog, Throwable throwable, ErrorContext context) { + logger.error("Failed to read " + errorLog + " deleting file"); + try { + Files.deleteIfExists(errorLog); + } catch (IOException ioException) { + logger.error("Failed to delete " + errorLog); + } + logNew(errorLog, throwable, context); + } + + private int getOccurrences(List lines) { + String occurLine = lines.get(0); + return Integer.parseInt(StringUtils.splitByWholeSeparator(occurLine, ": ")[2].trim()); + } + + private int findFirstContextLine(List lines, int lineCount) { + int firstContextLineIndex = 0; + for (int i = 0; i < lineCount; i++) { + if (lines.get(i).contains("---- Context")) { + firstContextLineIndex = i; + break; + } + } + return firstContextLineIndex; + } + + private void logToConsole(L level, Path errorLog, Throwable throwable, ErrorContext context) { + String errorName = throwable.getClass().getSimpleName(); + String errorMsg = throwable.getMessage(); + String errorLocation = errorLog.toString(); + logger.log(level, + "Ran into " + errorName + " - logged to " + errorLocation, + "(INCLUDE CONTENTS OF THE FILE IN ANY REPORTS)", + context.getWhatToDo().map(td -> "What to do: " + td).orElse("Error msg: \"" + errorMsg + "\"") + ); + } + + private void logNew(Path errorLog, Throwable throwable, ErrorContext context) { + List stacktrace = buildReadableStacktrace(throwable); + List lines = Lists.builder(String.class) + .add("Last occurred: " + getTimeStamp() + " Occurrences: 1") + .apply(builder -> this.buildContext(context, 1, builder)) + .add("---- Stacktrace ----") + .addAll(stacktrace) + .build(); + writeNew(errorLog, throwable, lines); + } + + private void writeNew(Path errorLog, Throwable throwable, List lines) { + try { + Files.createDirectories(errorLog.getParent()); + Files.write(errorLog, lines, StandardOpenOption.CREATE_NEW, StandardOpenOption.APPEND); + } catch (IOException e) { + throwable.addSuppressed(e); + Logger.getGlobal().log(Level.SEVERE, "Failed to log Plan error, see suppressed.", throwable); + } + } + + @Override + @Deprecated + public void log(L level, Class caughtBy, Throwable throwable) { + log(level, throwable, ErrorContext.builder() + .related("Caught by " + caughtBy.getName()) + .build()); + } + + private String hash(Throwable e) { + StringBuilder seed = new StringBuilder(); + Throwable cause = e; + Set alreadyPresent = new HashSet<>(); + while (cause != null) { + for (StackTraceElement element : cause.getStackTrace()) { + String asLine = element.toString(); + if (!alreadyPresent.contains(asLine)) { + seed.append(asLine); + } + alreadyPresent.add(asLine); + } + cause = e.getCause(); + } + return DigestUtils.sha256Hex(seed.toString()).substring(0, 10); + } + + private List buildReadableStacktrace(Throwable e) { + List trace = new ArrayList<>(); + trace.add(e.toString()); + for (StackTraceElement element : e.getStackTrace()) { + trace.add(" " + element); + } + Throwable cause = e.getCause(); + if (cause != null) { + trace.add("Caused by:"); + trace.addAll(buildReadableStacktrace(cause)); + } + return trace; + } +} diff --git a/Plan/common/src/main/resources/assets/plan/bungeeconfig.yml b/Plan/common/src/main/resources/assets/plan/bungeeconfig.yml index aac82e73e..c1f1aafb4 100644 --- a/Plan/common/src/main/resources/assets/plan/bungeeconfig.yml +++ b/Plan/common/src/main/resources/assets/plan/bungeeconfig.yml @@ -15,7 +15,7 @@ Plugin: Create_new_locale_file_on_next_enable: false Debug: false Dev: false - Delete_logs_after_days: 2 + Delete_logs_after_days: 7 Update_notifications: # Display update notification on the website Check_for_updates: true diff --git a/Plan/common/src/main/resources/assets/plan/config.yml b/Plan/common/src/main/resources/assets/plan/config.yml index 5fa864180..30be966d4 100644 --- a/Plan/common/src/main/resources/assets/plan/config.yml +++ b/Plan/common/src/main/resources/assets/plan/config.yml @@ -14,7 +14,7 @@ Plugin: Create_new_locale_file_on_next_enable: false Debug: false Dev: false - Delete_logs_after_days: 2 + Delete_logs_after_days: 7 Update_notifications: # Display update notification on the website Check_for_updates: true