Merge pull request #84 from faststats-dev/errors

Improve error tracking

Author: NonSwag

bukkit/example-plugin/src/main/java/com/example/ExamplePlugin.java

@@ -2,7 +2,6 @@
 
 import dev.faststats.bukkit.BukkitMetrics;
 import dev.faststats.core.ErrorTracker;
-import dev.faststats.core.Metrics;
 import dev.faststats.core.chart.Chart;
 import org.bukkit.plugin.java.JavaPlugin;
 
@@ -15,7 +14,7 @@ public class ExamplePlugin extends JavaPlugin {
     // context-unaware error tracker, does not automatically track errors
     public static final ErrorTracker CONTEXT_UNAWARE_ERROR_TRACKER = ErrorTracker.contextUnaware();
 
-    private final Metrics metrics = BukkitMetrics.factory()
+    private final BukkitMetrics metrics = BukkitMetrics.factory()
             .url(URI.create("https://metrics.example.com/v1/collect")) // For self-hosted metrics servers only
 
             // Custom example charts
@@ -36,9 +35,14 @@ public class ExamplePlugin extends JavaPlugin {
             .token("YOUR_TOKEN_HERE") // required -> token can be found in the settings of your project
             .create(this);
 
+    @Override
+    public void onEnable() {
+        metrics.ready(); // register additional error handlers
+    }
+
     @Override
     public void onDisable() {
-        metrics.shutdown();
+        metrics.shutdown(); // safely shut down metrics submission
     }
 
     public void doSomethingWrong() {

bukkit/src/main/java/dev/faststats/bukkit/BukkitMetrics.java

@@ -1,6 +1,7 @@
 package dev.faststats.bukkit;
 
 import dev.faststats.core.Metrics;
+import org.bukkit.plugin.IllegalPluginAccessException;
 import org.bukkit.plugin.Plugin;
 import org.jetbrains.annotations.Contract;
 
@@ -21,6 +22,18 @@ static Factory factory() {
         return new BukkitMetricsImpl.Factory();
     }
 
+    /**
+     * Registers additional exception handlers on Paper-based implementations.
+     *
+     * @throws IllegalPluginAccessException if the plugin is not yet enabled
+     * @apiNote This method may only be called {@link Plugin#onEnable() onEnable()}.
+     * @since 0.14.0
+     */
+    @Override
+    void ready() throws IllegalPluginAccessException;
+
     interface Factory extends Metrics.Factory<Plugin, Factory> {
+        @Override
+        BukkitMetrics create(Plugin object) throws IllegalStateException;
     }
 }

bukkit/src/main/java/dev/faststats/bukkit/BukkitMetricsImpl.java

@@ -1,9 +1,7 @@
 package dev.faststats.bukkit;
 
 import com.google.gson.JsonObject;
-import dev.faststats.core.Metrics;
 import dev.faststats.core.SimpleMetrics;
-import org.bukkit.Server;
 import org.bukkit.plugin.Plugin;
 import org.jetbrains.annotations.Async;
 import org.jetbrains.annotations.Contract;
@@ -13,11 +11,9 @@
 import java.util.Optional;
 import java.util.function.Supplier;
 import java.util.logging.Level;
-import java.util.logging.Logger;
 
 final class BukkitMetricsImpl extends SimpleMetrics implements BukkitMetrics {
-    private final Logger logger;
-    private final Server server;
+    private final Plugin plugin;
 
     private final String pluginVersion;
     private final String minecraftVersion;
@@ -29,8 +25,8 @@ final class BukkitMetricsImpl extends SimpleMetrics implements BukkitMetrics {
     private BukkitMetricsImpl(final Factory factory, final Plugin plugin, final Path config) throws IllegalStateException {
         super(factory, config);
 
-        this.logger = plugin.getLogger();
-        this.server = plugin.getServer();
+        this.plugin = plugin;
+        var server = plugin.getServer();
 
         this.pluginVersion = tryOrEmpty(() -> plugin.getPluginMeta().getVersion())
                 .orElseGet(() -> plugin.getDescription().getVersion());
@@ -42,14 +38,20 @@ private BukkitMetricsImpl(final Factory factory, final Plugin plugin, final Path
         startSubmitting();
     }
 
+    Plugin plugin() {
+        return plugin;
+    }
+
     private boolean checkOnlineMode() {
+        var server = plugin.getServer();
         return tryOrEmpty(() -> server.getServerConfig().isProxyOnlineMode())
                 .or(() -> tryOrEmpty(this::isProxyOnlineMode))
                 .orElseGet(server::getOnlineMode);
     }
 
     @SuppressWarnings("removal")
     private boolean isProxyOnlineMode() {
+        var server = plugin.getServer();
         final var proxies = server.spigot().getPaperConfig().getConfigurationSection("proxies");
         if (proxies == null) return false;
 
@@ -72,7 +74,7 @@ protected void appendDefaultData(final JsonObject charts) {
 
     private int getPlayerCount() {
         try {
-            return server.getOnlinePlayers().size();
+            return plugin.getServer().getOnlinePlayers().size();
         } catch (final Throwable t) {
             error("Failed to get player count", t);
             return 0;
@@ -81,17 +83,26 @@ private int getPlayerCount() {
 
     @Override
     protected void printError(final String message, @Nullable final Throwable throwable) {
-        logger.log(Level.SEVERE, message, throwable);
+        plugin.getLogger().log(Level.SEVERE, message, throwable);
     }
 
     @Override
     protected void printInfo(final String message) {
-        logger.info(message);
+        plugin.getLogger().info(message);
     }
 
     @Override
     protected void printWarning(final String message) {
-        logger.warning(message);
+        plugin.getLogger().warning(message);
+    }
+
+    @Override
+    public void ready() {
+        if (getErrorTracker().isPresent()) try {
+            Class.forName("com.destroystokyo.paper.event.server.ServerExceptionEvent");
+            plugin.getServer().getPluginManager().registerEvents(new PaperEventListener(this), plugin);
+        } catch (final ClassNotFoundException ignored) {
+        }
     }
 
     private <T> Optional<T> tryOrEmpty(final Supplier<T> supplier) {
@@ -104,7 +115,7 @@ private <T> Optional<T> tryOrEmpty(final Supplier<T> supplier) {
 
     static final class Factory extends SimpleMetrics.Factory<Plugin, BukkitMetrics.Factory> implements BukkitMetrics.Factory {
         @Override
-        public Metrics create(final Plugin plugin) throws IllegalStateException {
+        public BukkitMetrics create(final Plugin plugin) throws IllegalStateException {
             final var dataFolder = getPluginsFolder(plugin).resolve("faststats");
             final var config = dataFolder.resolve("config.properties");
             return new BukkitMetricsImpl(this, plugin, config);

bukkit/src/main/java/dev/faststats/bukkit/PaperEventListener.java

@@ -0,0 +1,16 @@
+package dev.faststats.bukkit;
+
+import com.destroystokyo.paper.event.server.ServerExceptionEvent;
+import com.destroystokyo.paper.exception.ServerPluginException;
+import org.bukkit.event.EventHandler;
+import org.bukkit.event.EventPriority;
+import org.bukkit.event.Listener;
+
+record PaperEventListener(BukkitMetricsImpl metrics) implements Listener {
+    @EventHandler(priority = EventPriority.MONITOR)
+    public void onServerException(final ServerExceptionEvent event) {
+        if (!(event.getException() instanceof final ServerPluginException exception)) return;
+        if (!exception.getResponsiblePlugin().equals(metrics.plugin())) return;
+        metrics.getErrorTracker().ifPresent(tracker -> tracker.trackError(exception));
+    }
+}

bungeecord/example-plugin/src/main/java/com/example/ExamplePlugin.java

@@ -38,7 +38,7 @@ public class ExamplePlugin extends Plugin {
 
     @Override
     public void onDisable() {
-        metrics.shutdown();
+        metrics.shutdown(); // safely shut down metrics submission
     }
 
     public void doSomethingWrong() {

core/src/main/java/dev/faststats/core/ErrorHelper.java

@@ -6,7 +6,10 @@
 
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
+import java.util.IdentityHashMap;
 import java.util.List;
+import java.util.Set;
 
 final class ErrorHelper {
     private static final int MESSAGE_LENGTH = Math.min(1000, Integer.getInteger("faststats.message-length", 500));
@@ -104,18 +107,25 @@ private static List<String> collapseConsecutiveDuplicates(final List<String> lin
     }
 
     public static boolean isSameLoader(final ClassLoader loader, final Throwable error) {
+        return isSameLoader(loader, error, Collections.newSetFromMap(new IdentityHashMap<>()));
+    }
+
+    private static boolean isSameLoader(final ClassLoader loader, @Nullable final Throwable error, final Set<Throwable> visited) {
+        if (error == null || !visited.add(error)) return false;
+
         final var stackTrace = error.getStackTrace();
-        if (stackTrace == null || stackTrace.length == 0) return false;
+        if (stackTrace == null || stackTrace.length == 0)
+            return isSameLoader(loader, error.getCause(), visited);
 
         final var firstNonLibraryIndex = findFirstNonLibraryFrameIndex(stackTrace);
-        if (firstNonLibraryIndex == -1) return false;
+        if (firstNonLibraryIndex == -1) return isSameLoader(loader, error.getCause(), visited);
 
         final var framesToCheck = Math.min(5, stackTrace.length - firstNonLibraryIndex);
 
         for (var i = 0; i < framesToCheck; i++) {
             final var frame = stackTrace[firstNonLibraryIndex + i];
             if (isLibraryClass(frame.getClassName())) continue;
-            if (!isFromLoader(frame, loader)) return false;
+            if (!isFromLoader(frame, loader)) return isSameLoader(loader, error.getCause(), visited);
         }
 
         return true;

core/src/main/java/dev/faststats/core/ErrorTracker.java

@@ -163,4 +163,17 @@ static ErrorTracker contextUnaware() {
      */
     @Contract(pure = true)
     TrackingThreadPoolExecutor threadPoolExecutor();
+
+    /**
+     * Checks if the error occurred in the same class loader as the provided loader.
+     *
+     * @param loader the class loader
+     * @param error  the error
+     * @return whether the error occurred in the same class loader
+     * @since 0.14.0
+     */
+    @Contract(pure = true)
+    static boolean isSameLoader(ClassLoader loader, Throwable error) {
+        return ErrorHelper.isSameLoader(loader, error);
+    }
 }    

core/src/main/java/dev/faststats/core/Metrics.java

@@ -43,7 +43,19 @@ public interface Metrics {
     Config getConfig();
 
     /**
-     * Shuts down the metrics submission.
+     * Performs additional post-startup tasks.
+     * <p>
+     * This method may only be called when the application startup is complete.
+     * <p>
+     * <i>No-op in most implementations.</i>
+     *
+     * @since 0.14.0
+     */
+    default void ready() {
+    }
+
+    /**
+     * Safely shuts down the metrics submission.
      * <p>
      * This method should be called when the application is shutting down.
      *

core/src/main/java/dev/faststats/core/SimpleErrorTracker.java

@@ -89,7 +89,7 @@ public synchronized void attachErrorContext(@Nullable final ClassLoader loader)
             final var handler = originalHandler;
             if (handler != null) handler.uncaughtException(thread, error);
             try {
-                if (loader != null && !ErrorHelper.isSameLoader(loader, error)) return;
+                if (loader != null && !ErrorTracker.isSameLoader(loader, error)) return;
                 final var event = errorEvent;
                 if (event != null) event.accept(loader, error);
                 trackError(error);