840118 - Switch error reporting over to ErrorMill

Status: RESOLVED FIXED

(Webtools Graveyard :: Elmo, defect, P2)

Description

[Peter Bengtsson [:peterbe]]

Arecibo is going away and Sentry (errormill.mozilla.org) is the new cool kid on the block.

Comment 1

[Peter Bengtsson [:peterbe]]

Attachment: less arecibo more raven

Comment 2

[Peter Bengtsson [:peterbe]]

I have a URL to put into SENTRY_URL on dev and prod. I'll do that when I deploy.

Comment 3

[Axel Hecht [:Pike]]

Comment on attachment 717978 [details] [diff] [review]
less arecibo more raven

Review of attachment 717978 [details] [diff] [review]:
-----------------------------------------------------------------

Can you explain how the test setup works? I failed to find how SENTRY_URL makes it through to switch off tests and development environments.

Also, I'd rip out everything we did to customize the 404 handler, if sentry doesn't need that. Including tests and all.

Sadly, it seems that we still need to overload the 500 handler, as that doesn't come with a RequestContext. I wonder if we can create the next template in a way that let's us drop the requestcontext data for 500 errors.

Canceling review request 'til I get a better picture.

Comment 4

[Peter Bengtsson [:peterbe]]

Attachment: fixes from review

Comment 5

[Axel Hecht [:Pike]]

Comment on attachment 718560 [details] [diff] [review]
fixes from review

Review of attachment 718560 [details] [diff] [review]:
-----------------------------------------------------------------

I'm still confused how the whole setup works. Can you outline how which settings actually lead to raven doing something? So far, I only see that we're hooking up the app, which imports a bunch of stuff, but I don't see how it actually instantiates anything.

I also have comments on details, the custom 404 handler isn't referenced anymore, you can also remove the no-op impl of it. And I wonder if the test settings should just be in settings.base.

Cancelling the review again, as I really don't know if something's not hooked up in my brain or in the code.

::: apps/homepage/views.py
@@ +26,1 @@
>      return page_not_found(request)

we should remove the view here completely, too.

::: settings_test.py
@@ +25,5 @@
>  )
> +
> +# don't accidentally do anything whilst running tests
> +RAVEN_CONFIG = {}
> +SENTRY_DSN = None

I wonder if these should be in settings.base.

Comment 6

[Peter Bengtsson [:peterbe]]

Attachment: removed unused handler404 function

I'll respond to the other comments as a reply.

Comment 7

[Peter Bengtsson [:peterbe]]

(In reply to Axel Hecht [:Pike] from comment #5)
> Comment on attachment 718560 [details] [diff] [review]
> fixes from review
> 
> Review of attachment 718560 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm still confused how the whole setup works. Can you outline how which
> settings actually lead to raven doing something? So far, I only see that
> we're hooking up the app, which imports a bunch of stuff, but I don't see
> how it actually instantiates anything.
> 
Here is where the magic happens:
https://github.com/getsentry/raven-python/blob/master/raven/contrib/django/models.py#L180

Because it's in `INSTALLED_APPS` its models.py gets imported automatically. 
So this happens:
https://github.com/getsentry/raven-python/blob/master/raven/contrib/django/models.py#L202

> I also have comments on details, the custom 404 handler isn't referenced
> anymore, you can also remove the no-op impl of it. And I wonder if the test
> settings should just be in settings.base.
> 
> Cancelling the review again, as I really don't know if something's not
> hooked up in my brain or in the code.
> 
> ::: apps/homepage/views.py
> @@ +26,1 @@
> >      return page_not_found(request)
> 
> we should remove the view here completely, too.
> 
Oops! Thanks. Gone now. 

> ::: settings_test.py
> @@ +25,5 @@
> >  )
> > +
> > +# don't accidentally do anything whilst running tests
> > +RAVEN_CONFIG = {}
> > +SENTRY_DSN = None
> 
> I wonder if these should be in settings.base.

No. The reason they're in settings_test.py is because that's always the last thing that gets imported and it trumps everything. We don't want failing unit tests to accidentally send out things to sentry. 

Note, the most correct way of specifying a DSN for sentry is to use the `RAVEN_CONFIG` dict and setting it in there. Or you can just define a `SENTRY_DSN` variable. By setting both to voids here in settings_test.py we're just double-guarding ourselves from the risk. 

Does that help?

Comment 8

[Axel Hecht [:Pike]]

Comment on attachment 719052 [details] [diff] [review]
removed unused handler404 function

Review of attachment 719052 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good.

I wonder if we want to keep the test for submission around or if that's cool to drop? It is a pretty expensive test in terms of setting up the logic, and it only tests a small portion of the actual submission process if we mock raven code early. And is really costly if we don't (i.e., try to fake a http response).

I'm fine with dropping the test, but drop the
from mock import patch
in tests.py, too. r=me with that.

I quickly glanced, we're still using mock in lib/auth/tests.py, so the dev requirement should stay.

Comment 9

[[github robot]]

Commit pushed to develop at https://github.com/mozilla/elmo

https://github.com/mozilla/elmo/commit/9f9e293ef99a5c3a67d1545fd585deaad77facb7
bug 840118 - errormill, r=Pike