862618 - Initial review of node-bugzilla

Status: RESOLVED WONTFIX

(Webmaker Graveyard :: DevOps, defect)

Description

[David Humphrey (:humph)]

Attachment: https://github.com/humphd/node-bugzilla

I wrote a node module to automatically file bugs in bugzilla when a node app crashes.  You can see an example bug created with it here:

https://landfill.bugzilla.org/bzapi_sandbox/show_bug.cgi?id=11547

I'm not sure if I need to do anything more to have it fit in well with Express apps or if this method is enough.

Once you're happy with it, I'd like to set it up for all our apps so we get good info on any crashes we see.

Comment 1

[Jon Buckley]

Comment on attachment 738278 [details]
https://github.com/humphd/node-bugzilla

Some code comments:

* Instead of writing your own stack parsing functions, just use one of the modules on npm: https://npmjs.org/search?q=stack. Although the stack-parsing code is so small, possible not worth it.
* Using domains instead of catching the exception with uncaughtException would be the new, cool node way of handling exceptions.
* I'd give it a more accurate name, like bugzilla-crash-reporter.
* It'd be nice to add functionality for sending non-fatal errors.
* I think you need to call "process.exit()" after handling the error, according to the node API docs

I couldn't get this to send bugzilla error reports when it was located inside an express route. I was only able to get it to send an error when I used setTimeout().

A larger question is whether we want to implement this for our applications, and I wouldn't want to. Newrelic already provides error collection/alerting for us, and I think we should just stick with that: https://rpm.newrelic.com/accounts/255689/applications/2703518/traced_errors

Comment 2

[David Humphrey (:humph)]

> * Instead of writing your own stack parsing functions, just use one of the
> modules on npm: https://npmjs.org/search?q=stack. Although the stack-parsing
> code is so small, possible not worth it.

There isn't much that needs to get parsed, tbh.  Nothing in that list really helps me do what I'm not already doing.

> * Using domains instead of catching the exception with uncaughtException
> would be the new, cool node way of handling exceptions.

Using domains in general across all our apps is a perfectly legit suggestion.  It would be worth filing bugs on that.

It doesn't really change the intent of this bug, though.  No matter the mechanism, we'll still have top-level errors we care about seeing.

> * I'd give it a more accurate name, like bugzilla-crash-reporter.

Good idea.

> * It'd be nice to add functionality for sending non-fatal errors.

You mean something like `bugzilla.bug( 'details' )`?  I want something like this for doing crash reports via the browser, and having those get filed too.

> * I think you need to call "process.exit()" after handling the error,
> according to the node API docs

Probably doing domains above negates the need for this, but I'll research.

> I couldn't get this to send bugzilla error reports when it was located
> inside an express route. I was only able to get it to send an error when I
> used setTimeout().

I wondered if error middleware would eat things.  Probably I also need a middleware bugzillaErrorHandler too?

> A larger question is whether we want to implement this for our applications,
> and I wouldn't want to. Newrelic already provides error collection/alerting
> for us, and I think we should just stick with that:
> https://rpm.newrelic.com/accounts/255689/applications/2703518/traced_errors

Here's my beef with things like newrelic: only a small number of people will ever see it.  Take our Popcorn Maker crashes right now, which go to S3.  No one looks at them because they aren't in our open workflow.  That link you gave above, it requires login.  Right there it's a failure in my book.

If I had bugs coming in for app crashes, I'd have people fixing them.

Comment 3

[Jon Buckley]

> You mean something like `bugzilla.bug( 'details' )`?  I want something like this for doing crash reports via the browser, and having those get filed too.

Not even the browser, something like:

try {
  null.b();
} catch (ex) {
  bugzilla.reportError(ex);
}

> Probably doing domains above negates the need for this, but I'll research.

I think the idea behind killing the process is that your program is in an undefined state. variables may be partially initialized, that sort of thing.

> I wondered if error middleware would eat things.  Probably I also need a middleware bugzillaErrorHandler too?

Yeah, that might be it. bugzilla.expressErrorHandler() ?

> Here's my beef with things like newrelic: only a small number of people will ever see it.  Take our Popcorn Maker crashes right now, which go to S3.  No one looks at them because they aren't in our open workflow.  That link you gave above, it requires login.  Right there it's a failure in my book.

So visibility is a problem that needs to be solved. I don't think the problem here is New Relic though; it's client crashes that are stored on S3. You don't get any sort of data aggregation, you need to check everything manually. Sentry ( https://getsentry.com/welcome/ ) would be good for client-side error aggregation. But that's another ticket!

> If I had bugs coming in for app crashes, I'd have people fixing them.

You'd have people looking for other bugs to fix :)

We simply don't have app crashes often enough. If the server is crashing, we know because in the browser you'll get HTTP 500 errors (from Zeus in the past, we should figure out this behaviour with Amazon ELB's) when trying to save, publish or load. JP also some monitoring setup that will ring alarm bells when things stop working. Then we fix it, deploy it, and everything's fine until the next time.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Dave: is this something you're actually going to pick up and do? If not, might want to close it out as invalid or wontfix.