Remove unnecessary methods for legacy error reporting

TEAMMATES · Apr 16, 2022 · 0ec7310 · 0ec7310
1 parent 5f16eff
commit 0ec7310
Show file tree

Hide file tree

Showing 13 changed files with 202 additions and 90 deletions.
diff --git a/src/main/java/teammates/common/datatransfer/ErrorLogEntry.java b/src/main/java/teammates/common/datatransfer/ErrorLogEntry.java
@@ -1,5 +1,13 @@
 package teammates.common.datatransfer;
 
+import java.util.List;
+
+import teammates.common.datatransfer.logs.ExceptionLogDetails;
+import teammates.common.datatransfer.logs.GeneralLogEntry;
+import teammates.common.datatransfer.logs.LogEvent;
+import teammates.common.datatransfer.logs.LogSeverity;
+import teammates.common.util.JsonUtils;
+
 /**
  * Represents an error level entry from the logs.
  */
@@ -14,6 +22,51 @@ public ErrorLogEntry(String message, String severity, String traceId) {
         this.traceId = traceId;
     }
 
+    /**
+     * Converts a {@link GeneralLogEntry} to a condensed {@link ErrorLogEntry}.
+     */
+    public static ErrorLogEntry fromLogEntry(GeneralLogEntry logEntry) {
+        assert logEntry.getSeverity().getSeverityLevel() >= LogSeverity.ERROR.getSeverityLevel();
+
+        String message;
+        if (logEntry.getDetails() == null) {
+            message = logEntry.getMessage();
+        } else if (logEntry.getDetails().getEvent() != LogEvent.EXCEPTION_LOG) {
+            message = JsonUtils.toJson(logEntry.getDetails());
+        } else {
+            ExceptionLogDetails exceptionLog = (ExceptionLogDetails) logEntry.getDetails();
+            StringBuilder sb = new StringBuilder();
+            sb.append(exceptionLog.getMessage()).append('\n');
+
+            List<String> exceptionClasses = exceptionLog.getExceptionClasses();
+            List<String> exceptionMessages = exceptionLog.getExceptionMessages();
+            List<List<String>> exceptionStackTraces = exceptionLog.getExceptionStackTraces();
+
+            // This is a set of extra-defensive checks. A well-formed exception log should not need these checks
+            // as exception messages are not null and all the list sizes are equal.
+            int numIterations = Math.min(exceptionClasses.size(), exceptionStackTraces.size());
+            if (exceptionMessages != null) {
+                numIterations = Math.min(numIterations, exceptionMessages.size());
+            }
+
+            for (int i = 0; i < numIterations; i++) {
+                sb.append("caused by ").append(exceptionClasses.get(i));
+                if (exceptionMessages != null) {
+                    sb.append(": ").append(exceptionMessages.get(i));
+                }
+                sb.append('\n');
+                List<String> stackTraces = exceptionStackTraces.get(i);
+                for (String stackTrace : stackTraces) {
+                    sb.append("    at ").append(stackTrace).append('\n');
+                }
+            }
+            message = sb.toString();
+        }
+        String severity = logEntry.getSeverity().toString();
+        String traceId = logEntry.getTrace();
+        return new ErrorLogEntry(message, severity, traceId);
+    }
+
     public String getMessage() {
         return message;
     }

diff --git a/src/main/java/teammates/common/util/EmailWrapper.java b/src/main/java/teammates/common/util/EmailWrapper.java
@@ -105,10 +105,4 @@ public void setIsCopy(boolean isCopy) {
         this.isCopy = isCopy;
     }
 
-    public String getInfoForLogging() {
-        return "[Email sent]to=" + getRecipient()
-               + "|from=" + getSenderEmail()
-               + "|subject=" + getSubject();
-    }
-
 }
diff --git a/src/main/java/teammates/logic/api/EmailGenerator.java b/src/main/java/teammates/logic/api/EmailGenerator.java
@@ -1004,7 +1004,7 @@ private String generateSevereErrorLogLine(int index, String logMessage, String l
                 EmailTemplates.SEVERE_ERROR_LOG_LINE,
                 "${index}", String.valueOf(index),
                 "${errorType}", logLevel,
-                "${errorMessage}", logMessage,
+                "${errorMessage}", logMessage.replaceAll("\n", "\n<br>"),
                 "${traceId}", traceId);
     }
 

diff --git a/src/main/java/teammates/logic/api/EmailSender.java b/src/main/java/teammates/logic/api/EmailSender.java
@@ -90,16 +90,4 @@ private boolean isTestingAccount(String email) {
         return email.endsWith(Const.TEST_EMAIL_DOMAIN);
     }
 
-    /**
-     * Sends the given {@code report}.
-     */
-    public void sendReport(EmailWrapper report) {
-        try {
-            sendEmail(report);
-        } catch (Exception e) {
-            log.severe("Error in sending report: " + (report == null ? "" : report.getInfoForLogging())
-                       + "\nReport content: " + (report == null ? "" : report.getContent()), e);
-        }
-    }
-
 }
diff --git a/src/main/java/teammates/logic/api/LogsProcessor.java b/src/main/java/teammates/logic/api/LogsProcessor.java
@@ -3,7 +3,6 @@
 import java.time.Instant;
 import java.util.List;
 
-import teammates.common.datatransfer.ErrorLogEntry;
 import teammates.common.datatransfer.FeedbackSessionLogEntry;
 import teammates.common.datatransfer.QueryLogsResults;
 import teammates.common.datatransfer.logs.GeneralLogEntry;
@@ -37,13 +36,6 @@ public static LogsProcessor inst() {
         return instance;
     }
 
-    /**
-     * Gets the list of recent error- or higher level logs.
-     */
-    public List<ErrorLogEntry> getRecentErrorLogs() {
-        return service.getRecentErrorLogs();
-    }
-
     /**
      * Queries and retrieves logs with given parameters.
      */

diff --git a/src/main/java/teammates/logic/external/GoogleCloudLoggingService.java b/src/main/java/teammates/logic/external/GoogleCloudLoggingService.java
@@ -16,7 +16,6 @@
 import com.google.cloud.logging.Payload;
 import com.google.cloud.logging.Severity;
 
-import teammates.common.datatransfer.ErrorLogEntry;
 import teammates.common.datatransfer.FeedbackSessionLogEntry;
 import teammates.common.datatransfer.QueryLogsResults;
 import teammates.common.datatransfer.logs.FeedbackSessionAuditLogDetails;
@@ -43,25 +42,6 @@ public class GoogleCloudLoggingService implements LogService {
 
     private static final String TRACE_PREFIX = String.format("projects/%s/traces/", Config.APP_ID);
 
-    @Override
-    public List<ErrorLogEntry> getRecentErrorLogs() {
-        Instant endTime = Instant.now();
-        // Sets the range to 6 minutes to slightly overlap the 5 minute email timer
-        long queryRange = 1000 * 60 * 6;
-        Instant startTime = endTime.minusMillis(queryRange);
-
-        QueryLogsParams queryLogsParams = QueryLogsParams.builder(startTime.toEpochMilli(), endTime.toEpochMilli())
-                .withMinSeverity(LogSeverity.ERROR)
-                .build();
-
-        List<ErrorLogEntry> errorLogs = new ArrayList<>();
-        for (GeneralLogEntry logEntry : queryLogs(queryLogsParams).getLogEntries()) {
-            String message = logEntry.getDetails() == null ? logEntry.getMessage() : JsonUtils.toJson(logEntry.getDetails());
-            errorLogs.add(new ErrorLogEntry(message, logEntry.getSeverity().toString(), logEntry.getTrace()));
-        }
-        return errorLogs;
-    }
-
     @Override
     public QueryLogsResults queryLogs(QueryLogsParams queryLogsParams) {
 

diff --git a/src/main/java/teammates/logic/external/LocalLoggingService.java b/src/main/java/teammates/logic/external/LocalLoggingService.java
@@ -16,7 +16,6 @@
 import com.google.common.reflect.TypeToken;
 import com.google.gson.JsonParseException;
 
-import teammates.common.datatransfer.ErrorLogEntry;
 import teammates.common.datatransfer.FeedbackSessionLogEntry;
 import teammates.common.datatransfer.QueryLogsResults;
 import teammates.common.datatransfer.logs.ExceptionLogDetails;
@@ -66,12 +65,6 @@ private static List<GeneralLogEntry> loadLocalLogEntries() {
         }
     }
 
-    @Override
-    public List<ErrorLogEntry> getRecentErrorLogs() {
-        // Not supported in dev server
-        return new ArrayList<>();
-    }
-
     @Override
     public QueryLogsResults queryLogs(QueryLogsParams queryLogsParams) {
         // Page size is set as a small value to test loading of more logs

diff --git a/src/main/java/teammates/logic/external/LogService.java b/src/main/java/teammates/logic/external/LogService.java
@@ -2,7 +2,6 @@
 
 import java.util.List;
 
-import teammates.common.datatransfer.ErrorLogEntry;
 import teammates.common.datatransfer.FeedbackSessionLogEntry;
 import teammates.common.datatransfer.QueryLogsResults;
 import teammates.common.datatransfer.logs.QueryLogsParams;
@@ -12,11 +11,6 @@
  */
 public interface LogService {
 
-    /**
-     * Gets the list of recent error- or higher level logs.
-     */
-    List<ErrorLogEntry> getRecentErrorLogs();
-
     /**
      * Gets the list of logs satisfying the given criteria.
      */

diff --git a/src/main/java/teammates/ui/webapi/CompileLogsAction.java b/src/main/java/teammates/ui/webapi/CompileLogsAction.java
@@ -1,8 +1,13 @@
 package teammates.ui.webapi;
 
+import java.time.Instant;
+import java.util.ArrayList;
 import java.util.List;
 
 import teammates.common.datatransfer.ErrorLogEntry;
+import teammates.common.datatransfer.logs.GeneralLogEntry;
+import teammates.common.datatransfer.logs.LogSeverity;
+import teammates.common.datatransfer.logs.QueryLogsParams;
 import teammates.common.util.EmailWrapper;
 
 /**
@@ -12,17 +17,27 @@ class CompileLogsAction extends AdminOnlyAction {
 
     @Override
     public JsonResult execute() {
-        List<ErrorLogEntry> errorLogs = logsProcessor.getRecentErrorLogs();
-        sendEmail(errorLogs);
-        return new JsonResult("Successful");
-    }
+        Instant endTime = Instant.now();
+        // Sets the range to 6 minutes to slightly overlap the 5 minute email timer
+        long queryRange = 1000 * 60 * 6;
+        Instant startTime = endTime.minusMillis(queryRange);
+
+        QueryLogsParams queryLogsParams = QueryLogsParams.builder(startTime.toEpochMilli(), endTime.toEpochMilli())
+                .withMinSeverity(LogSeverity.ERROR)
+                .withPageSize(0)
+                .build();
+
+        List<ErrorLogEntry> errorLogs = new ArrayList<>();
+        for (GeneralLogEntry logEntry : logsProcessor.queryLogs(queryLogsParams).getLogEntries()) {
+            errorLogs.add(ErrorLogEntry.fromLogEntry(logEntry));
+        }
 
-    private void sendEmail(List<ErrorLogEntry> logs) {
         // Do not send any emails if there are no severe logs; prevents spamming
-        if (!logs.isEmpty()) {
-            EmailWrapper message = emailGenerator.generateCompiledLogsEmail(logs);
-            emailSender.sendReport(message);
+        if (!errorLogs.isEmpty()) {
+            EmailWrapper message = emailGenerator.generateCompiledLogsEmail(errorLogs);
+            emailSender.sendEmail(message);
         }
+        return new JsonResult("Successful");
     }
 
 }
diff --git a/src/test/java/teammates/common/datatransfer/ErrorLogEntryTest.java b/src/test/java/teammates/common/datatransfer/ErrorLogEntryTest.java
@@ -0,0 +1,102 @@
+package teammates.common.datatransfer;
+
+import java.time.Instant;
+import java.util.HashMap;
+import java.util.List;
+
+import org.testng.annotations.Test;
+
+import teammates.common.datatransfer.logs.ExceptionLogDetails;
+import teammates.common.datatransfer.logs.GeneralLogEntry;
+import teammates.common.datatransfer.logs.InstanceLogDetails;
+import teammates.common.datatransfer.logs.LogSeverity;
+import teammates.common.datatransfer.logs.SourceLocation;
+import teammates.test.BaseTestCase;
+
+/**
+ * SUT: {@link ErrorLogEntry}.
+ */
+public class ErrorLogEntryTest extends BaseTestCase {
+
+    @Test
+    public void testFromLogEntry_noLogDetails_shouldGetTextPayload() {
+        GeneralLogEntry logEntry = createTypicalLogEntry();
+        logEntry.setMessage("Test message");
+
+        ErrorLogEntry errorLogEntry = ErrorLogEntry.fromLogEntry(logEntry);
+        assertEquals("Test message", errorLogEntry.getMessage());
+        assertEquals("ERROR", errorLogEntry.getSeverity());
+        assertEquals("traceid", errorLogEntry.getTraceId());
+    }
+
+    @Test
+    public void testFromLogEntry_logDetailsNotException_shouldGetSerializedPayload() {
+        InstanceLogDetails instanceLogDetails = new InstanceLogDetails();
+        instanceLogDetails.setInstanceId("instanceid123");
+        instanceLogDetails.setInstanceEvent("STARTUP");
+
+        GeneralLogEntry logEntry = createTypicalLogEntry();
+        logEntry.setDetails(instanceLogDetails);
+
+        ErrorLogEntry errorLogEntry = ErrorLogEntry.fromLogEntry(logEntry);
+        assertEquals("{\n"
+                + "  \"instanceId\": \"instanceid123\",\n"
+                + "  \"instanceEvent\": \"STARTUP\",\n"
+                + "  \"event\": \"INSTANCE_LOG\"\n"
+                + "}", errorLogEntry.getMessage());
+        assertEquals("ERROR", errorLogEntry.getSeverity());
+        assertEquals("traceid", errorLogEntry.getTraceId());
+    }
+
+    @Test
+    public void testFromLogEntry_exceptionLogDetails_shouldGetPrettyPrintedLog() {
+        ExceptionLogDetails exceptionLogDetails = new ExceptionLogDetails();
+        exceptionLogDetails.setMessage("ActionMappingException caught by WebApiServlet");
+        exceptionLogDetails.setExceptionClasses(List.of(
+                "teammates.common.exception.ActionMappingException"
+        ));
+        exceptionLogDetails.setExceptionMessages(List.of(
+                "Resource with URI /webapi/404 is not found."
+        ));
+        exceptionLogDetails.setExceptionStackTraces(List.of(
+                List.of(
+                        "teammates.ui.webapi.ActionFactory.getAction(ActionFactory.java:168)",
+                        "teammates.ui.webapi.ActionFactory.getAction(ActionFactory.java:163)",
+                        "teammates.ui.webapi.WebApiServlet.invokeServlet(WebApiServlet.java:67)",
+                        "teammates.ui.webapi.WebApiServlet.doGet(WebApiServlet.java:44)",
+                        "javax.servlet.http.HttpServlet.service(HttpServlet.java:687)",
+                        "javax.servlet.http.HttpServlet.service(HttpServlet.java:790),",
+                        "javax.servlet.http.HttpServlet.service(HttpServlet.java:790),",
+                        "..."
+                )
+        ));
+
+        GeneralLogEntry logEntry = createTypicalLogEntry();
+        logEntry.setDetails(exceptionLogDetails);
+
+        String expectedMessage = String.join("\n", List.of(
+                "ActionMappingException caught by WebApiServlet",
+                "caused by teammates.common.exception.ActionMappingException: Resource with URI /webapi/404 is not found.",
+                "    at teammates.ui.webapi.ActionFactory.getAction(ActionFactory.java:168)",
+                "    at teammates.ui.webapi.ActionFactory.getAction(ActionFactory.java:163)",
+                "    at teammates.ui.webapi.WebApiServlet.invokeServlet(WebApiServlet.java:67)",
+                "    at teammates.ui.webapi.WebApiServlet.doGet(WebApiServlet.java:44)",
+                "    at javax.servlet.http.HttpServlet.service(HttpServlet.java:687)",
+                "    at javax.servlet.http.HttpServlet.service(HttpServlet.java:790),",
+                "    at javax.servlet.http.HttpServlet.service(HttpServlet.java:790),",
+                "    at ...",
+                ""
+        ));
+
+        ErrorLogEntry errorLogEntry = ErrorLogEntry.fromLogEntry(logEntry);
+        assertEquals(expectedMessage, errorLogEntry.getMessage());
+        assertEquals("ERROR", errorLogEntry.getSeverity());
+        assertEquals("traceid", errorLogEntry.getTraceId());
+    }
+
+    private GeneralLogEntry createTypicalLogEntry() {
+        return new GeneralLogEntry(LogSeverity.ERROR, "traceid", "insertid", new HashMap<>(),
+                new SourceLocation("file1", 100L, "func1"), Instant.now().toEpochMilli());
+    }
+
+}
diff --git a/src/test/java/teammates/logic/api/MockLogsProcessor.java b/src/test/java/teammates/logic/api/MockLogsProcessor.java
@@ -4,7 +4,6 @@
 import java.util.HashMap;
 import java.util.List;
 
-import teammates.common.datatransfer.ErrorLogEntry;
 import teammates.common.datatransfer.FeedbackSessionLogEntry;
 import teammates.common.datatransfer.QueryLogsResults;
 import teammates.common.datatransfer.logs.GeneralLogEntry;
@@ -18,17 +17,9 @@
  */
 public class MockLogsProcessor extends LogsProcessor {
 
-    private List<ErrorLogEntry> errorLogs = new ArrayList<>();
     private List<FeedbackSessionLogEntry> feedbackSessionLogs = new ArrayList<>();
     private List<GeneralLogEntry> generalLogs = new ArrayList<>();
 
-    /**
-     * Simulates insertion of error logs.
-     */
-    public void insertErrorLog(String message, String severity, String traceId) {
-        errorLogs.add(new ErrorLogEntry(message, severity, traceId));
-    }
-
     /**
      * Simulates insertion of feedback session logs.
      */
@@ -37,11 +28,6 @@ public void insertFeedbackSessionLog(String studentEmail, String feedbackSession
         feedbackSessionLogs.add(new FeedbackSessionLogEntry(studentEmail, feedbackSessionName, fslType, timestamp));
     }
 
-    @Override
-    public List<ErrorLogEntry> getRecentErrorLogs() {
-        return errorLogs;
-    }
-
     /**
      * Simulates insertion of general INFO logs.
      */
@@ -63,7 +49,7 @@ public void insertWarningLog(String trace, String insertId, SourceLocation sourc
     /**
      * Simulates insertion of general ERROR logs.
      */
-    public void insertGeneralErrorLog(String trace, String insertId, SourceLocation sourceLocation,
+    public void insertErrorLog(String trace, String insertId, SourceLocation sourceLocation,
             long timestamp, String textPayloadMessage, LogDetails logDetails) {
         insertGeneralLog(LogSeverity.ERROR, trace, insertId,
                 sourceLocation, timestamp, textPayloadMessage, logDetails);