Create Bugsnag reports when errors are encountered #108

evansiroky · 2018-08-06T21:56:54Z

The goal of this PR is to try to catch all possible halt-inducing exceptions that result in an HTTP >= 500 error and create Bugsnag reports from the exception.

landonreed · 2018-08-07T16:49:51Z

@evansiroky this PR has changes for for many of the old editor controllers (e.g., /src/main/java/com/conveyal/datatools/editor/controllers/api/AgencyController.java). Instead of updating the halts, we might should just remove these classes altogether because they are no longer in use and, unlike the editor model classes, they are not needed for migrating from the MapDB-backed editor.

evansiroky · 2018-08-07T16:59:46Z

@landonreed I don't know which classes those are. Would you be able to remove them in a separate PR and then I can build on top of that?

landonreed

Generally looks good. Just a few changes needed. Although I do have a mild concern about having to pass the request object to the haltWithMessage method, simply because it requires passing the request to downstream methods that might need to halt (e.g., downloadFile). So, I wonder if another approach might keep things a bit tighter or maybe we just remove the halt from downloadFile and always wrap it in a try/catch (throwing the halt in the catch).

src/main/java/com/conveyal/datatools/manager/controllers/api/FeedVersionController.java

src/main/java/com/conveyal/datatools/manager/controllers/api/GtfsPlusController.java

src/main/java/com/conveyal/datatools/manager/controllers/api/NoteController.java

src/main/java/com/conveyal/datatools/manager/controllers/api/OrganizationController.java

src/main/java/com/conveyal/datatools/manager/jobs/MergeProjectFeedsJob.java

codecov-io · 2018-08-08T18:15:13Z

Codecov Report

Merging #108 into dev will increase coverage by 0.02%.
The diff coverage is 1.11%.

@@            Coverage Diff             @@
##               dev    #108      +/-   ##
==========================================
+ Coverage     5.15%   5.17%   +0.02%     
- Complexity      93      94       +1     
==========================================
  Files          133     133              
  Lines         6872    6875       +3     
  Branches       903     905       +2     
==========================================
+ Hits           354     356       +2     
- Misses        6490    6491       +1     
  Partials        28      28

Impacted Files	Coverage Δ	Complexity Δ
...conveyal/datatools/manager/models/FeedVersion.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
.../conveyal/datatools/manager/models/FeedSource.java	`6.14% <0%> (ø)`	`3 <0> (ø)`	⬇️
...nveyal/datatools/manager/codec/LocalDateCodec.java	`20% <0%> (-1.43%)`	`3 <0> (ø)`
...ols/manager/controllers/api/ProjectController.java	`12.9% <0%> (-0.22%)`	`2 <0> (ø)`
...atools/manager/controllers/api/UserController.java	`7.77% <0%> (ø)`	`2 <0> (ø)`	⬇️
...xtensions/transitland/TransitLandFeedResource.java	`6.32% <0%> (-0.09%)`	`3 <0> (ø)`
...ensions/transitfeeds/TransitFeedsFeedResource.java	`7.5% <0%> (-0.1%)`	`3 <0> (ø)`
...va/com/conveyal/datatools/manager/ConvertMain.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
.../com/conveyal/datatools/editor/jobs/GisExport.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
.../manager/controllers/api/FeedSourceController.java	`8.72% <0%> (-0.31%)`	`2 <0> (ø)`
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d22da45...33eb392. Read the comment docs.

landonreed

Just a few changes/comments that need addressing. Also, could you update the PR description to better describe the changes?

src/main/java/com/conveyal/datatools/common/utils/S3Utils.java

src/main/java/com/conveyal/datatools/common/utils/SparkUtils.java

src/main/java/com/conveyal/datatools/manager/persistence/FeedStore.java

landonreed · 2018-11-05T20:26:12Z

src/main/java/com/conveyal/datatools/manager/persistence/FeedStore.java

@@ -202,66 +214,45 @@ public File getFeed (String id) {
    /**
     * Create a new feed with the given ID.


I feel like this method doc needs a little more info about what it does.

I'm actually not sure what this does. Help?

This is obviously legacy code, so maybe it's outside the scope of your changes, but since you touched the method it made me notice that (to me) the javadoc doesn't really describe at all what the method does especially since the storeFeedLocally method has been removed from the internals. I would suggest that it say something like:

Store GTFS file locally. This method is used when a new feed version or generated GTFS file (e.g., the product of merging multiple GTFS files from a project) needs to be stored locally for future use. Note: uploading the file to S3 is handled elsewhere as a finishing step, e.g., at the conclusion of a successful feed processing/validation step.

src/main/java/com/conveyal/datatools/manager/controllers/DumpController.java

evansiroky · 2018-11-08T01:24:16Z

See questions in unresolved conversations. Otherwise this should be good to go.

landonreed · 2018-11-08T15:20:12Z

Thanks @evansiroky. I just addressed the questions.

evansiroky · 2018-11-08T23:17:09Z

Comments added, ready to merge?

landonreed

Hi @evansiroky, sorry for the late response on this. Upon further review, I have a few more comments/changes and it looks like it needs to be updated to fix merge conflicts as well.

src/main/java/com/conveyal/datatools/common/utils/SparkUtils.java

src/main/java/com/conveyal/datatools/editor/controllers/api/EditorController.java

landonreed · 2018-12-11T21:03:20Z

src/main/java/com/conveyal/datatools/editor/utils/JacksonSerializers.java

                return date;
            } catch (Exception jsonException) {
                // This is here to catch any loads of database dumps that happen to have the old java.util.Date
                // field type in validationResult.  God help us.
-                System.out.println("Error parsing date value, trying legacy java.util.Date date format");
+                LOG.warn(
+                    String.format("Error parsing date value: `%s`, trying legacy java.util.Date date format", dateText)


LOG.warn has string substitution if you use {}:

LOG.warn("Error parsing date value: `{}`, trying legacy java.util.Date date format", dateText)

landonreed · 2018-12-11T21:03:56Z

src/main/java/com/conveyal/datatools/editor/utils/JacksonSerializers.java

                try {
                    date = Instant.ofEpochMilli(jp.getValueAsLong()).atZone(ZoneOffset.UTC).toLocalDate();
                    return date;
                } catch (Exception e) {
+                    LOG.warn(
+                        String.format("Error parsing date value: `%s`", dateText)


See above comment about string substitution in LOG.warn.

landonreed · 2018-12-11T21:13:26Z

src/main/java/com/conveyal/datatools/manager/controllers/api/GtfsPlusController.java

            LOG.error("Unable to open input stream from upload");
-            haltWithMessage(req, 500, "an unexpected error occurred", e);
+            haltWithMessage(req, 400, "Unable to open input stream from upload", e);


Related to the early comment about having haltWithMessage also log messages. This would keep things DRY-er here.

landonreed · 2018-12-11T21:16:46Z

src/main/java/com/conveyal/datatools/manager/controllers/api/ProjectController.java

@@ -140,7 +138,7 @@ private static Project deleteProject(Request req, Response res) {
        Project project = requestProjectById(req, "manage");
        boolean successfullyDeleted = project.delete();
        if (!successfullyDeleted) {
-            haltWithMessage(req, 400, "Did not delete project.");
+            haltWithMessage(req, 500, "Did not delete project.", new Exception("Delete unsuccessful"));


It doesn't seem great to be just creating arbitrary exceptions here that are not really tied back to the "failing" code. Do we gain anything by having a bugsnag report that points back here?

I don't think it is possible to create a Bugsnag report without an exception. So I think what we'd gain here is receiving a Bugsnag report that the user received a 500 error from the server.

landonreed · 2018-12-11T21:17:45Z

src/main/java/com/conveyal/datatools/manager/controllers/api/ProjectController.java

+            try {
+                DataManager.feedResources.get(syncType).importFeedsForProject(proj, req.headers("Authorization"));
+            } catch (Exception e) {
+                haltWithMessage(req, 500, "An error occurred while trying to sync", e);


Isn't this PR partially intended to remove generic exceptions? Does this hinder its purpose?

The most important part of this PR is the creation of bugsnag reports whenever a 500 error halt occurs. Wherever possible I think it's good to be as specific as possible to create better error reports, but I think it's fine to catch generic exceptions too. In this specific case, the abstract class method only specifies the Exception class as something that is thrown.

landonreed · 2018-12-11T21:18:36Z

src/main/java/com/conveyal/datatools/manager/controllers/api/ProjectController.java

@@ -360,6 +365,7 @@ public static ScheduledFuture scheduleAutoFeedFetch (Project project, int interv
            return DataManager.scheduler.scheduleAtFixedRate(fetchProjectFeedsJob,
                    delayInMinutes, TimeUnit.DAYS.toMinutes(intervalInDays), minutes);
        } catch (Exception e) {
+            LOG.error("Failed to schedule auto feed fetch");


Should this register an error with bugsnag?

Sure why not?

landonreed · 2018-12-11T21:19:26Z

...main/java/com/conveyal/datatools/manager/extensions/transitland/TransitLandFeedResource.java

@@ -74,8 +76,8 @@ public void importFeedsForProject(Project project, String authHeader) {
                con.setRequestProperty("User-Agent", "User-Agent");

                int responseCode = con.getResponseCode();
-                System.out.println("\nSending 'GET' request to URL : " + url);
-                System.out.println("Response Code : " + responseCode);
+                LOG.info("\nSending 'GET' request to URL : " + url);


You can remove this new line char.

also a few other things to address PR comments

evansiroky · 2018-12-12T22:34:04Z

comments should be addressed, merge conflicts resolved.

landonreed · 2018-12-19T15:20:51Z

src/main/java/com/conveyal/datatools/common/utils/SparkUtils.java

        Request request,
        int statusCode,
        String message,
        Exception e
    ) throws HaltException {
+        // Note that halting occurred, also print error stacktrace if applicable
+        if (e != null) e.printStackTrace();
+        LOG.info("Halting with status code {}", statusCode);


@evansiroky, thanks for this change, but I was originally thinking that we would also log the message arg unconditionally as well (i.e., combining the log statement at line 115 with this log statement). I feel like logging just the statusCode leaves out some info that could be helpful in debugging.

landonreed

Thanks for the changes @evansiroky. I just have one comment: https://github.com/catalogueglobal/datatools-server/pull/108/files/28288340e9f0d6e9bea364584d6a63af628cd721..b61fb9b71b429d9be1ecfa64190bca1ba0d90e25#r242955193

evansiroky · 2018-12-20T23:17:50Z

Message added to log statement.

fix #108

evansiroky requested a review from landonreed August 8, 2018 06:57

landonreed requested changes Aug 8, 2018

View reviewed changes

evansiroky added 2 commits August 8, 2018 21:26

refactor(logging): notify bugsnag of unexpected errors

c10236b

refactor(logging): improve error logging

9ad87ee

evansiroky mentioned this pull request Aug 12, 2018

Error logging limited #112

Merged

evansiroky added 2 commits August 11, 2018 20:26

Merge branch 'error-logging-limited' into error-logging

60e7221

Merge branch 'error-logging-limited' into error-logging

c20e76c

landonreed self-assigned this Oct 10, 2018

landonreed assigned evansiroky and unassigned landonreed Nov 5, 2018

landonreed requested changes Nov 5, 2018

View reviewed changes

Merge branch 'dev' into error-logging

e24e376

evansiroky changed the title ~~Add better logging of server-side errors~~ Create Bugsnag reports when errors are encountered Nov 7, 2018

evansiroky added 3 commits November 7, 2018 17:00

refactor(utils): merge 500 error halting into haltWithMessage

324f030

refactor(log): include logback.xml file in builds

d254198

refactor(bugsnag): pin bugsnag to version 3.3.0

b68944f

evansiroky assigned landonreed and unassigned evansiroky Nov 8, 2018

landonreed mentioned this pull request Nov 8, 2018

Change request/response logging level to debug #122

Open

landonreed assigned evansiroky and unassigned landonreed Nov 8, 2018

docs(feedstore): add more comments to newFeed method

2828834

evansiroky assigned landonreed and unassigned evansiroky Nov 8, 2018

This was referenced Dec 11, 2018

Create Bugsnag reports when errors are encountered ibi-group/datatools-server#127

Closed

Change request/response logging level to debug ibi-group/datatools-server#141

Open

landonreed requested changes Dec 11, 2018

View reviewed changes

landonreed assigned evansiroky and unassigned landonreed Dec 11, 2018

evansiroky added 2 commits December 12, 2018 12:32

Merge branch 'dev' into error-logging

19b6ad6

refactor(logging): rename haltWithMessage to logMessageAndHalt

b61fb9b

also a few other things to address PR comments

evansiroky assigned landonreed and unassigned evansiroky Dec 12, 2018

landonreed reviewed Dec 19, 2018

View reviewed changes

landonreed assigned evansiroky and unassigned landonreed Dec 19, 2018

refactor(halts): include error message string in halting log statement

33eb392

evansiroky assigned landonreed and unassigned evansiroky Dec 20, 2018

landonreed approved these changes Jan 7, 2019

View reviewed changes

landonreed merged commit ecdc62a into dev Jan 7, 2019

This was referenced Jan 7, 2019

Merge back changes to Conveyal repo ibi-group/datatools-server#163

Merged

Release: fix maven-semantic-release by including commits for v3.2.0 ibi-group/datatools-server#164

Merged

landonreed added a commit that referenced this pull request Aug 7, 2019

build(pom): set maven-jar-plugin to v3.1.2 to silence warning

cc1e67f

fix #108

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create Bugsnag reports when errors are encountered #108

Create Bugsnag reports when errors are encountered #108

evansiroky commented Aug 6, 2018 •

edited

Loading

landonreed commented Aug 7, 2018 •

edited

Loading

evansiroky commented Aug 7, 2018

landonreed left a comment

codecov-io commented Aug 8, 2018 •

edited

Loading

landonreed left a comment •

edited

Loading

landonreed Nov 5, 2018

evansiroky Nov 8, 2018

landonreed Nov 8, 2018

evansiroky commented Nov 8, 2018

landonreed commented Nov 8, 2018

evansiroky commented Nov 8, 2018

landonreed left a comment

landonreed Dec 11, 2018

landonreed Dec 11, 2018

landonreed Dec 11, 2018

landonreed Dec 11, 2018

evansiroky Dec 12, 2018

landonreed Dec 11, 2018

evansiroky Dec 12, 2018

landonreed Dec 11, 2018

evansiroky Dec 12, 2018

landonreed Dec 11, 2018

evansiroky commented Dec 12, 2018

landonreed Dec 19, 2018

landonreed left a comment

evansiroky commented Dec 20, 2018

		@@ -202,66 +214,45 @@ public File getFeed (String id) {
		/**
		* Create a new feed with the given ID.

Create Bugsnag reports when errors are encountered #108

Create Bugsnag reports when errors are encountered #108

Conversation

evansiroky commented Aug 6, 2018 • edited Loading

landonreed commented Aug 7, 2018 • edited Loading

evansiroky commented Aug 7, 2018

landonreed left a comment

Choose a reason for hiding this comment

codecov-io commented Aug 8, 2018 • edited Loading

Codecov Report

landonreed left a comment • edited Loading

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

evansiroky commented Nov 8, 2018

landonreed commented Nov 8, 2018

evansiroky commented Nov 8, 2018

landonreed left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evansiroky commented Dec 12, 2018

Choose a reason for hiding this comment

landonreed left a comment

Choose a reason for hiding this comment

evansiroky commented Dec 20, 2018

evansiroky commented Aug 6, 2018 •

edited

Loading

landonreed commented Aug 7, 2018 •

edited

Loading

codecov-io commented Aug 8, 2018 •

edited

Loading

landonreed left a comment •

edited

Loading