ControllerServlet: do not sent email to admin upon error #6587 #6796

LiHaoTan · 2017-03-12T03:51:54Z

Outline of Solution

Error report is no longer sent by email immediately.
Add comments to explain why an error report will not be sent.

Additional Info
I added the comments as I feel the code does not explain why an error report does not need to be sent through email. Not sure if this is appropriate.

I also worked on this issue because the person originally working on the issue KYUUBI have went missing.

damithc · 2017-03-12T04:08:36Z

src/main/java/teammates/ui/controller/ControllerServlet.java

+             */
+            if (errorReport != null) {
+                log.severe(ActivityLogEntry.generateSystemErrorReportLogMessage(req, errorReport, userType));
+            }


As we are trying to save time here (we have less than 1 second to finish this part), perhaps just a simple log.severe("Unexpected exception " + e.getMessage()) is enough, in which case we can avoid lines 126-134 too?

Ok I have removed the generating of error report.

damithc · 2017-03-12T04:35:35Z

src/main/java/teammates/ui/controller/ControllerServlet.java

+             * in GAE shutting down the instance.
+             * Note that severe logs are sent by email automatically in the cron job auto/compileLogs.
+             */
+            log.severe("Unexpected exception " + t.getMessage());


Good work. While you are at it, see if there is a way to include the stack trace in the log message. Use TeammatesException#toStringWithStackTrace(...)?

damithc · 2017-03-12T04:44:16Z

src/main/java/teammates/ui/controller/ControllerServlet.java

@@ -126,7 +127,7 @@ public final void doPost(HttpServletRequest req, HttpServletResponse resp) throw
             * in GAE shutting down the instance.
             * Note that severe logs are sent by email automatically in the cron job auto/compileLogs.
             */
-            log.severe("Unexpected exception " + t.getMessage());
+            log.severe(TeammatesException.toStringWithStackTrace(t));


Better. Can you also include a"Unexpected exception caught by ControllerServlet : " ? I would like an easy way to identify log messages originating from this location.

Ok I have added that. However, there seems to be a failing test unrelated to what I have changed. I have added more details in the conversation.

LiHaoTan · 2017-03-12T05:50:20Z

Running the TestNG test for AccountsDbTest.java results in a failure if testGetAccount is run before testEditAccount.

The failed test can be seen here: https://gist.github.com/anonymous/65a51785dcc12cc9880f5ecfb364cdfd

So if the order of the test changes it will work, for example the following test suite will work:
https://gist.github.com/anonymous/6b08ec022a187d5199a180c2149d9202

Additional Info

Usual sequence of working test:
ci-tests > component-tests > teammates.test.cases.storage.AccountsDbTest.testCreateAccount PASSED
ci-tests > component-tests > teammates.test.cases.storage.AccountsDbTest.testDeleteAccount PASSED
ci-tests > component-tests > teammates.test.cases.storage.AccountsDbTest.testEditAccount PASSED
ci-tests > component-tests > teammates.test.cases.storage.AccountsDbTest.testGetAccount PASSED

Sequence for failing test:
ci-tests > component-tests > teammates.test.cases.storage.AccountsDbTest.testDeleteAccount PASSED
ci-tests > component-tests > teammates.test.cases.storage.AccountsDbTest.testGetAccount FAILED
java.lang.AssertionError at AccountsDbTest.java:35
ci-tests > component-tests > teammates.test.cases.storage.AccountsDbTest.testEditAccount PASSED
ci-tests > component-tests > teammates.test.cases.storage.AccountsDbTest.testCreateAccount PASSED

xpdavid · 2017-03-12T08:24:17Z

@LiHaoTan Yes. The test cases for *LogicTest will fail randomly. Should be find after #6778.

LiHaoTan · 2017-03-12T12:56:37Z

@xpdavid Thanks for the info. The tests are now passing.

wkurniawan07 · 2017-03-12T14:33:12Z

@damithc doing what is done right now will strip off all the information originally sent via email: request method, request user agent, request path, request URL, request parameters. I believe at least three of them are useful, if not critical, in troubleshooting.

damithc · 2017-03-12T15:52:51Z

@damithc doing what is done right now will strip off all the information originally sent via email: request method, request user agent, request path, request URL, request parameters. I believe at least three of them are useful, if not critical, in troubleshooting.

This PR is doing what I have been doing for many months now. I've been applying this "fix" to live code before deploying ever since we started having many server crashes. This PR is just migrating my hot patch to the permanent code base. :-p

Yes, it will strip off useful info from the log message, but those info are logged by GAE anyway, so they are not completely lost. It's just a bit harder to locate. I'm not sure how much we can do within 1 second under full load, so I don't want to risk entire server being killed in order to collect those info.

Having said that, we'll try to add those things back after we've resolved some of our other performance issues and server crashes are no longer a big concern.

wkurniawan07 · 2017-03-12T16:17:37Z

In that case, @LiHaoTan check if the following functions are still used somewhere else:

EmailGenerator#generateSystemErrorEmail
ActivityLogEntry#generateSystemErrorReportLogMessage

If they are not, remove them as well.

Remove ActivityLogEntry#generateSystemErrorReportLogMessage. Remove EmailGenerator#generateSystemErrorEmail. Remove test case EmailGeneratorTest#testSystemCrashReportEmailContent.

LiHaoTan · 2017-03-12T16:51:41Z

Ok I have removed them. By the way, they still look useful, or are they not?

wkurniawan07 · 2017-03-13T15:47:10Z

@LiHaoTan useful as it may, retaining an unused method for that reason is a mindset that causes dead code to stay in a code base. Its effect is non-trivial; you can read about it here.

wkurniawan07

Few more things to remove.

wkurniawan07 · 2017-03-13T15:51:29Z

src/test/java/teammates/test/cases/logic/EmailGeneratorTest.java

-        String subject = String.format(EmailType.ADMIN_SYSTEM_ERROR.getSubject(),
-                                       Config.getAppVersion(), error.getMessage());
-
-        verifyEmail(email, Config.SUPPORT_EMAIL, subject, "/systemCrashReportEmail.html");


This HTML file should be fine to be removed. It can be found in the folder src/test/resources/emails.

wkurniawan07 · 2017-03-13T15:51:37Z

src/test/java/teammates/test/cases/logic/EmailGeneratorTest.java

-        modifiedContent = email.getContent().replaceAll(lastCommonLineRegex, "$1...$2");
-        email.setContent(modifiedContent);
-
-        verifyEmail(email, Config.SUPPORT_EMAIL, subject, "/systemCrashReportEmailLessInfo.html");


Similarly here.

LiHaoTan · 2017-03-13T16:22:19Z

@wkurniawan07 Ok I have removed the files, sorry for overlooking them.

Thanks for the link on the dead code.

wkurniawan07

Few more things to remove.

wkurniawan07 · 2017-03-13T16:48:20Z

src/main/java/teammates/logic/api/EmailGenerator.java

-                "${stackTrace}", stackTrace);
-
-        EmailWrapper email = getEmptyEmailAddressedToEmail(Config.SUPPORT_EMAIL);
-        email.setSubject(String.format(EmailType.ADMIN_SYSTEM_ERROR.getSubject(), Config.getAppVersion(), errorMessage));


Looks like I missed this. This enum EmailType.ADMIN_SYSTEM_ERROR should be fine to be removed as well.

wkurniawan07 · 2017-03-13T16:49:48Z

src/main/java/teammates/logic/api/EmailGenerator.java

-
-        String actualUser = userType == null || userType.id == null ? "Not logged in" : userType.id;
-
-        String emailBody = Templates.populateTemplate(EmailTemplates.SYSTEM_ERROR,


So is EmailTemplates.SYSTEM_ERROR, and by extension src/main/resources/systemErrorEmailTemplate.html.

wkurniawan07

Looks good, thanks for your contribution @LiHaoTan! 👍

…rror (TEAMMATES#6796)

Remove sending of email to admin when error report is generated

e582e49

LiHaoTan changed the title ~~Remove sending of email to admin when error report is generated #6587~~ ControllerServlet: do not sent email to admin upon error #6587 Mar 12, 2017

damithc reviewed Mar 12, 2017

View reviewed changes

Remove generating of error report to make request terminate faster

211c5e2

damithc reviewed Mar 12, 2017

View reviewed changes

Change the logging to log the stack trace instead

0a92e58

damithc reviewed Mar 12, 2017

View reviewed changes

Update the log to include where it originates from

3e72089

Merge branch 'master' into 6587-remove-send-email

5f14fd6

xpdavid added the s.Ongoing The PR is being worked on by the author(s) label Mar 12, 2017

LiHaoTan added 2 commits March 13, 2017 00:44

Remove unused methods and tests for system error report

dd98468

Remove ActivityLogEntry#generateSystemErrorReportLogMessage. Remove EmailGenerator#generateSystemErrorEmail. Remove test case EmailGeneratorTest#testSystemCrashReportEmailContent.

Remove blank line in comment

4664ca4

Remove extra line break

492e9b1

wkurniawan07 requested changes Mar 13, 2017

View reviewed changes

wkurniawan07 self-assigned this Mar 13, 2017

Remove testSystemCrashReportEmailContent expected crash reports

11387e8

wkurniawan07 requested changes Mar 13, 2017

View reviewed changes

LiHaoTan added 2 commits March 14, 2017 00:56

Remove EmailTemplates.SYSTEM_ERROR and its associated file

76f73f3

Remove unused EmailType.ADMIN_SYSTEM_ERROR

e3c18b8

wkurniawan07 approved these changes Mar 13, 2017

View reviewed changes

wkurniawan07 added the s.FinalReview The PR is ready for final review label Mar 13, 2017

wkurniawan07 removed the s.Ongoing The PR is being worked on by the author(s) label Mar 13, 2017

damithc approved these changes Mar 14, 2017

View reviewed changes

damithc added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels Mar 14, 2017

Merge branch 'master' into 6587-remove-send-email

16c102f

wkurniawan07 merged commit d0e3918 into TEAMMATES:master Mar 14, 2017

wkurniawan07 added this to the V5.99 milestone Mar 14, 2017

LiHaoTan deleted the 6587-remove-send-email branch March 15, 2017 09:51

wkurniawan07 added the e.1 label Mar 15, 2017

shrut1996 pushed a commit to shrut1996/teammates that referenced this pull request Apr 15, 2017

[TEAMMATES#6587] ControllerServlet: do not sent email to admin upon e…

1c45ce1

…rror (TEAMMATES#6796)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ControllerServlet: do not sent email to admin upon error #6587 #6796

ControllerServlet: do not sent email to admin upon error #6587 #6796

LiHaoTan commented Mar 12, 2017

damithc Mar 12, 2017

LiHaoTan Mar 12, 2017

damithc Mar 12, 2017

damithc Mar 12, 2017

LiHaoTan Mar 12, 2017

LiHaoTan commented Mar 12, 2017 •

edited

Loading

xpdavid commented Mar 12, 2017

LiHaoTan commented Mar 12, 2017

wkurniawan07 commented Mar 12, 2017

damithc commented Mar 12, 2017

wkurniawan07 commented Mar 12, 2017

LiHaoTan commented Mar 12, 2017 •

edited

Loading

wkurniawan07 commented Mar 13, 2017

wkurniawan07 left a comment

wkurniawan07 Mar 13, 2017

wkurniawan07 Mar 13, 2017

LiHaoTan commented Mar 13, 2017 •

edited

Loading

wkurniawan07 left a comment

wkurniawan07 Mar 13, 2017

wkurniawan07 Mar 13, 2017

wkurniawan07 left a comment


		String actualUser = userType == null \|\| userType.id == null ? "Not logged in" : userType.id;

		String emailBody = Templates.populateTemplate(EmailTemplates.SYSTEM_ERROR,

ControllerServlet: do not sent email to admin upon error #6587 #6796

ControllerServlet: do not sent email to admin upon error #6587 #6796

Conversation

LiHaoTan commented Mar 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LiHaoTan commented Mar 12, 2017 • edited Loading

xpdavid commented Mar 12, 2017

LiHaoTan commented Mar 12, 2017

wkurniawan07 commented Mar 12, 2017

damithc commented Mar 12, 2017

wkurniawan07 commented Mar 12, 2017

LiHaoTan commented Mar 12, 2017 • edited Loading

wkurniawan07 commented Mar 13, 2017

wkurniawan07 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LiHaoTan commented Mar 13, 2017 • edited Loading

wkurniawan07 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wkurniawan07 left a comment

Choose a reason for hiding this comment

LiHaoTan commented Mar 12, 2017 •

edited

Loading

LiHaoTan commented Mar 12, 2017 •

edited

Loading

LiHaoTan commented Mar 13, 2017 •

edited

Loading