563110 - Tell user if a particular crash id/report is invalid or never submitted

Status: RESOLVED FIXED

(Socorro :: General, task)

Description

[[:Cww]]

Right now if a user gives us a "crash ID" that isn't valid, putting it into crash-stats just says it's fetching.  Check:

http://crash-stats.mozilla.com/report/pending/abcd1234-abcd-1234-abcd-123456100112

Since it's not ever going to return, can we figure this out in advance under the new architecture and just return an appropriate message.  (And something appropriate for the JSON API as well.)

This way, I don't keep users on hold telling them that I'm waiting for it and my API calls aren't using the "try 3 times over an hour and if it doesn't return, die" method of figuring it out.

Comment 1

[Brandon Savage [:brandon]]

Moving onto the 1.7.8 milestone.

Comment 2

[Brandon Savage [:brandon]]

Attachment: Middleware changes

This patch does a number of things:

* Allows priority jobs to be scheduled by the middleware
* Provides feedback to the webui as to whether or not the crash exists, either in raw or processed form
* Removes priority job scheduling from the webui
* Issues a 404 for invalid or unavailable OOIDs.

Comment 3

[K Lars Lohn [:lars] [:klohn]]

Comment on attachment 531712 [details] [diff] [review]
Middleware changes

To remain true to ideals of a RESTian API, I believe that SchedulePriorityJob should be a "put" not a "get".

I didn't think much about it earlier when looking at the GetCrash, but you're returning either False or True from 'fetchProcessed'.  Again, to remain true to a RESTian API shouldn't the boolean values be translated to 404 and 408 respectively?

Comment 4

[Brandon Savage [:brandon]]

Attachment: Patch v2

This patch improves upon the last one by returning appropriate status codes, and improving the REST handling in the webui. This REST code should eventually be replaced with a REST handler, but for now it does the job.

Comment 5

[Brandon Savage [:brandon]]

Attachment: Patch v3

Fixing a couple of issues that Lars raised with the patch overall.

Comment 6

[K Lars Lohn [:lars] [:klohn]]

Comment on attachment 532654 [details] [diff] [review]
Patch v3

the Python code looks fine.  My inspection of the PHP stuff was only cursory.

Comment 7

[Robert Helmer [:rhelmer]]

Comment on attachment 532654 [details] [diff] [review]
Patch v3

>Index: socorro/services/schedulePriorityJob.py
...
>+  def post(self, *args):


Everything else looks fine, I was a little confused about the above method was supposed to be called so tested it and found a couple issues, but they appear to be bugs in the existing socorro code base:

1) *args is not passed to self.post() in the web.py wrapper - therefore this code does not work via web.py (it's not hooked up in this patch, but I wanted to test it since the code is going in and would be presumed to work). Works locally if I just pass *args at http://code.google.com/p/socorro/source/browse/trunk/socorro/webapi/webapiService.py#58

2) stringLogger is forced in the web.py wrapper, so exceptions are eaten in mod_wsgi mode and apache logs "IOError: sys.stdout access restricted by mod_wsgi"

I'll file a separate bug for #2, but I'd suggest taking the simple fix to #1 as part of this patch.

Comment 8

[Brandon Savage [:brandon]]

This is fixed in revision 3165.

Comment 9

[Stephen Donner [:stephend] Not actively reading bugmail]

Attachment: Post-fix screenshot

Comment 10

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #9)
> Created attachment 533355 [details]
> Post-fix screenshot

...which seems wrong, given that it's returning a 200 OK.

We should look into this on https://crash-stats-dev.allizom.org, so I'm reopening.

Comment 11

[Brandon Savage [:brandon]]

Helped identify issue and assist WebQA with resolving it. Also made schedulePriorityJobs API public.

Comment 12

[Robert Helmer [:rhelmer]]

Comment on attachment 532654 [details] [diff] [review]
Patch v3

>Index: socorro/services/schedulePriorityJob.py
>===================================================================
>--- socorro/services/schedulePriorityJob.py	(revision 0)
>+++ socorro/services/schedulePriorityJob.py	(revision 0)
>@@ -0,0 +1,26 @@
>+import logging
>+logger = logging.getLogger("webapi")
>+
>+import socorro.lib.util as util
>+import socorro.webapi.webapiService as webapi
>+import socorro.database.database as db
>+
>+class SchedulePriorityJob(webapi.JsonServiceBase):
>+  def __init__(self, configContext):
>+    super(SchedulePriorityJob, self).__init__(configContext)
>+
>+  uri = '/201105/schedule/priority/job/(.*)'
>+
>+  def post(self, *args):
>+    convertedArgs = webapi.typeConversion([str], args)
>+    parameters = util.DotDict(zip(['uuid'], convertedArgs))
>+    connection = self.database.connection()
>+    sql = """INSERT INTO priorityjobs (uuid) VALUES (%s)"""
>+    try:
>+      db.execute(connection.cursor(), sql, (parameters['uuid'],))


I am testing this on crash-stats-dev (QA does not have access to the server so they can't hit the API and see the results directly like we can), and the above does not seem to actually be inserting to the priorityjobs table, although it runs and no exception is thrown.

If I spell it as:
connection.cursor().execute(sql, (parameters['uuid'],))
Then it does work.

I tried hooking up the LoggingCursor but it's not printing anything beyond "Now logging cursor".

Lars, let me know if anything jumps out at you, otherwise I'll do a bit more debugging on this.

Comment 13

[K Lars Lohn [:lars] [:klohn]]

if it is a critical fix to meet the deadline, then use your code that works. 

After that, investigate more deeply.  If you find a problem, file a bug for it.

Comment 14

[Robert Helmer [:rhelmer]]

(In reply to comment #13)
> if it is a critical fix to meet the deadline, then use your code that works. 
> 
> After that, investigate more deeply.  If you find a problem, file a bug for
> it.

I went ahead and checked in the workaround from comment 12:
Committed revision 3171.

Brandon is going to investigate more deeply and file a separate bug.

Comment 15

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #14)

> Brandon is going to investigate more deeply and file a separate bug.

I looked -- have this been yet filed?  Thanks!

Comment 16

[Robert Helmer [:rhelmer]]

(In reply to comment #15)
> (In reply to comment #14)
> 
> > Brandon is going to investigate more deeply and file a separate bug.
> 
> I looked -- have this been yet filed?  Thanks!

We discussed in irc earlier and I think we figured out the problem; it looks like the database.execute() wrapper isn't really intended for anything but select. It yields a while loop which does a cursor.fetchone(), and the database transaction doesn't seem to be finished until the generator is iterated over (cursor.fetchone() will throw an exception if done after insert, anyway).

I did test this on crash-stats-dev and it seems to work fine, I'll post results here. Just to clarify, the approach in comment 12 is what the database.execute() wrapper was doing behind the scenes, just without the yield that we just don't need in this case.

Comment 17

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #16)
> (In reply to comment #15)
 
> We discussed in irc earlier and I think we figured out the problem; it looks
> like the database.execute() wrapper isn't really intended for anything but
> select. It yields a while loop which does a cursor.fetchone(), and the
> database transaction doesn't seem to be finished until the generator is
> iterated over (cursor.fetchone() will throw an exception if done after
> insert, anyway).
> 
> I did test this on crash-stats-dev and it seems to work fine, I'll post
> results here. Just to clarify, the approach in comment 12 is what the
> database.execute() wrapper was doing behind the scenes, just without the
> yield that we just don't need in this case.

Can you post results here, Rob?  Thanks :-)

Comment 18

[Robert Helmer [:rhelmer]]

(In reply to comment #17)
> Can you post results here, Rob?  Thanks :-)

Sure, this is showing that the ooid is inserted into the priorityjobs table correctly:

"""
[[e-mail]1 conf.d]$ curl -X POST "http://socorro-api-dev-internal/bpapi/201105/schedule/priority/job/1234"
true
10.2.74.61 - - [27/May/2011:15:03:20 -0700] "POST /bpapi/201105/schedule/priority/job/1234 HTTP/1.1" 200 4 "-" "curl/7.19.7 (x86_64-redhat-linux-gnu) libcurl/7.19.7 NSS/3.12.6.2 zlib/1.2.3 libidn/1.18 libssh2/1.2.2"

[[e-mail]1 conf.d]$ psql -U $databaseUserName $database breakpad -c "select * from priorityjobs"
   uuid   
----------
 1234
(1 row)
"""

I have tested separately that inserting a valid ooid to the priorityjobs table results in priority processing happening (the test setup is a little more involved and this code hasn't changed). I can tell from the logs that it's trying (and failing) to process ooid "1234" which is expected in the above case.

Comment 19

[Robert Helmer [:rhelmer]]

Another note on comment 18 - this just a different way of calling the same method that gets called when an unprocessed ooid is put the reports page for example https://crash-stats-dev.allizom.org/report/index/1234

However the code on the reports page first checks to make sure the ooid is present in hbase and unprocessed before inserting into priorityjobs table.

Comment 20

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED per earlier testing, and comment 18/comment 19.  Thanks, Rob!

Comment 21

[Brandon Savage [:brandon]]

Attachment: Emergency patch to fix brokenness

This patch moves the check of the reports table to after we've identified that we have a valid report that's already been processed.

Comment 22

[Brandon Savage [:brandon]]

Committed the patch in revision 3188.

Comment 23

[Brandon Savage [:brandon]]

This issue has been corrected and released in version 1.7.8.1.