Closed Bug 563110 Opened 10 years ago Closed 9 years ago

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

Categories

(Socorro :: General, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: cww, Assigned: brandon)

References

()

Details

Attachments

(3 files, 2 obsolete files)

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.
Moving onto the 1.7.8 milestone.
Target Milestone: --- → 1.7.8
Assignee: nobody → bsavage
Attached patch Middleware changes (obsolete) — Splinter Review
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.
Attachment #531712 - Flags: review?(rhelmer)
Attachment #531712 - Flags: review?(lars)
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?
Attachment #531712 - Flags: review?(lars) → review-
Depends on: 656715
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #531712 - Attachment is obsolete: true
Attachment #532279 - Flags: review?(rhelmer)
Attachment #531712 - Flags: review?(rhelmer)
Attachment #532279 - Flags: review?(lars)
Attached patch Patch v3Splinter Review
Fixing a couple of issues that Lars raised with the patch overall.
Attachment #532279 - Attachment is obsolete: true
Attachment #532654 - Flags: review?(lars)
Attachment #532279 - Flags: review?(rhelmer)
Attachment #532279 - Flags: review?(lars)
Attachment #532654 - Flags: review?(rhelmer)
Comment on attachment 532654 [details] [diff] [review]
Patch v3

the Python code looks fine.  My inspection of the PHP stuff was only cursory.
Attachment #532654 - Flags: review?(lars) → review+
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.
Attachment #532654 - Flags: review?(rhelmer) → review+
This is fixed in revision 3165.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Helped identify issue and assist WebQA with resolving it. Also made schedulePriorityJobs API public.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
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.
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.
(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.
(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!
(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.
(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 :-)
(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:

"""
[rhelmer@socorro1.dev.dmz.sjc1 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"

[rhelmer@socorro1.dev.dmz.sjc1 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.
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.
Verified FIXED per earlier testing, and comment 18/comment 19.  Thanks, Rob!
Status: RESOLVED → VERIFIED
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.
Attachment #537163 - Flags: review?(rhelmer)
Attachment #537163 - Flags: feedback?(laura)
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Attachment #537163 - Flags: review?(rhelmer) → review+
Committed the patch in revision 3188.
This issue has been corrected and released in version 1.7.8.1.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Component: Socorro → General
Product: Webtools → Socorro
Duplicate of this bug: 579918
Attachment #537163 - Flags: feedback?(laura)
You need to log in before you can comment on or make changes to this bug.