584136 - Socorro UI - rework processed json fetching / priority job submission

Status: RESOLVED FIXED

(Socorro :: General, task)

Description

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

in 1.7, we use apache magik and business logic in the UI to handle priority jobs.  

In 1.8, the middleware will handle all the details and business logic.

The UI will have to make the processed json request using /201008/crash/processed/by/uuid**.  If the processed is not available, the middleware will return some token that means "not available try again later"*.  Meanwhile, behind the scenes, it will submit the priority job request which, hopefully will be handled in a timely manner.  Then later (30 seconds) the UI will make the same request again.  The middleware will either return the requested json or another try again later token.

We should also go all the way at this time and remove all database access from this function of the UI.  After successfully getting a processed json from the aforementioned middleware call, the UI should make another middleware call to /201008/crash/meta/by/uuid** to get the original metadata json to get the "sensitive" data.

* we'll need to talk about the token.  right now, I'm thinking that it will be just the word "no", but that isn't very creative.  Something containing words from http://github.com/fwenzel/reporter/blob/master/lib/swearwords/badwords.txt might be appropriate.

** the text of the uri is not set in stone.  I'm open to reworking it.  I don't like the word "meta".

Comment 1

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

It would ideal if the call would return an http status code that described the status.  However, there isn't an ideal status code that inherently states "not available try again later".  404 means "not found" but not "try again later".  202 Accepted (The request has been accepted for processing, but the processing has not been completed.) is the closest, but the definition just isn't inherent.

I think 1 word responses (although not creative as what is in badwords.txt) could work fine. "unavailable", "found", "processing", "gone"...

Some options for uris for the requests:
/201008/crashes/{ooid} - returns the raw dump of the crash
/201008/crashes/{ooid}/job - the details for the job that is queued for this crash (if still applicable)
/201008/crashes/{ooid}/processed - returns the full processed crash
/201008/crashes/{ooid}/details - returns the meta data of the crash

We should make sure that we standardize the terms that we decide to use in the uri, so that we are using that terminology throughout Socorro. (Note, we should ensure that http://code.google.com/p/socorro/wiki/SocorroGlossary is updated with these decisions)

Comment 2

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

the middleware api call is defined in .../socorro/services/getCrash.py

you can ask for one of three things: meta, dump or processed.

the one word response is only applicable to the degenerate case where a dump is not found.  If it is found, the call actually returns what you asked for, the meta data, the dump or the processed json.

Comment 3

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

ugh. I can see that we're really going to have to get the terminology straighten out.  That should have been: 'meta', 'raw_crash', 'processed'.

Comment 4

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

The UI currently submits jobs to the priorityjobs table in Postgres.  I'll yank out this code.  But will the API be returning any information about the priority job?  We currently display the priority job information on some pending crash report pages, and I need to know if I should account for that information.

Comment 5

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

Attachment: Patch for 584136

This is a machete patch.  I've hacked away at the code and cut out a decent amount of code.

Deleted the Dumps controller, and both Jobs models.  Moved most of the report logic from the Report model to the Crash library.  Cleaned up the Report controller to reduce method size.

Please refer to application/config/application.php-dist for changes made to the config file.

OOIDs available for testing are available at:
http://breakpad.pastebin.mozilla.org/766050

API pass-throughs are available at the following URLs.  "meta" and "raw_crash" require authentication.  "meta" and "processed" return json-encoded strings, while raw_crash returns a dump.

http://rsnyder.khan.mozilla.org/reporter/report/d235c795-f5ff-4cfd-b178-c718f2100811/meta
http://rsnyder.khan.mozilla.org/reporter/report/d235c795-f5ff-4cfd-b178-c718f2100811/processed
http://rsnyder.khan.mozilla.org/reporter/report/d235c795-f5ff-4cfd-b178-c718f2100811/raw_crash

I am not currently handling the various status codes (202, 404, 410) returned by the webservice layer but have left TODO notes to take care of that.  I'm hoping I can update the webservice layer in 1.9 to handle those codes and update all the calls throughout the site.

Comment 6

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

Attachment: Updated patch

This is an updated patch for 584136, which fixes issues with search caused by the refactoring.  Very minor changes, mainly setting methods in the Crash library from private to public.

Comment 7

[Austin King [:ozten]]

Comment on attachment 464963 [details] [diff] [review]
Patch for 584136

Great work! I think you rewrote the whole UI :)

I like that you've moved parseOOIDTimestamp into the Crash class.
controllers/report.php _APIAuthenticationRequired should return a boolean or void. It currently will do either depending on conditionals. You might want to do an exit if you want to make sure a bug won't allow access to something w/o authentication.     

_generateCrashesByOS - good cleanup

nit (I realize this is already in the codebase):
_OOID_410 - Remove hard coded 3 years from message (and the function validateOOID) and put in config. In general... this  is very specific to our Socorro install. I can see the value for an API result, but if this is accessed by users... It's probably overkill to do 410. This could be automated by finding the oldest crash in the system and giving a less specific message. "Crash not found, You request suggests a crash older than our system tracks"...

find - previously this function always redirected. Consider renaming it findOoidOrRedirect or validateOoidOrRedirect, or something similar to indicate that it does nothing on success and redirects on failure.

meta - Should check the return code of APIAuthenticationRequired or APIAuthenticationRequired should really use an exit. This is not secure. Perhaps rename it to APIAuthenticatOrDie.
Do we need to json_encode here, doesn't the hadoop API return JSON? Why trim the output?

raw - same feedback as meta

(In reply to comment #0)

pending - gets a little simpler, which is cool.

nit:
libraries/crash.php prepareCrashReport - whitespace issues (curly braces on next line only for functions)


prepareCrashReportsMeta - We need to capture (or return) the results of array_unique as it returns a new array.

Comment 8

[Austin King [:ozten]]

(In reply to comment #5)
Applied patch #2, having a tough time with those OOIDs.

I swear I requested 'bp-5f07f288-fa5a-4a61-a6f3-237da2100811' and got the page.
Then I tested report/index/ without the bp- prefix.
5f07f288-fa5a-4a61-a6f3-237da2100811 was a 404. Then I went back to the url
with 'bp-5f07f288-fa5a-4a61-a6f3-237da2100811' and that changed to a 404.

Can you confirm some good OOID for testing?
@lars - does the above sound like a possible API bug or just voodoo?

Comment 9

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

Austin - Per Lars in IRC, "lars: ryansnyder: I noticed that you submitted a topcrash request to my test middleware and it failed.  It looks like I managed to break every web service that touched the database.  I've corrected that problem."

But I'm still able to access those uuids in my sandbox, while I get 404 errors in your sandbox.  I'll keep trying to track down the issue.

Comment 10

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

Austin - could it be a cache problem?  In your sandbox, it seems like the first pull of an ooid of a processed crashed report works fine, but then it 404s afterwards.

Comment 11

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

Attachment: 3rd patch for 584136

Thanks Austin!  Here's another patch.  This patch obsoletes the prior 2 patches.  I know that you're still having issues, but I can't seem to replicate them in my sandbox.

* Added `$config['crash_report_ttl'] = 3;` to application.php for the number of years for a crash report's ttl.
* Updated _APIAuthenticationRequired() to exit if user is not authenticated.  This fixed meta() and raw_crash()
* Web_Service json_decodes results, so we have to re-encode them.  Not ideal, but didn't want to push yet another refactoring change into this patch.  I'd like to give Web_Service an overhaul in 1.9.
* Ditched Controller_Report::find()
* Crash::prepareCrashReport() fixed.

Comment 12

[Austin King [:ozten]]

Comment on attachment 465093 [details] [diff] [review]
3rd patch for 584136

Code looks good. You reason for not changing json_encode is a good one. Just wondering.

Comment 13

[Austin King [:ozten]]

(In reply to comment #11)
> Austin - could it be a cache problem?

Good call. I removed everything in my cache directory and I'm able to retrieve crashes. Sorry for the false alarm.

Comment 14

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

Thanks Austin.  Committing.  

==

Sending        webapp-php/application/config/application.php-dist
Sending        webapp-php/application/config/routes.php
Deleting       webapp-php/application/controllers/dumps.php
Sending        webapp-php/application/controllers/query.php
Sending        webapp-php/application/controllers/report.php
Sending        webapp-php/application/libraries/CrashReportDump.php
Sending        webapp-php/application/libraries/Web_Service.php
Sending        webapp-php/application/libraries/crash.php
Deleting       webapp-php/application/models/job.php
Deleting       webapp-php/application/models/priorityjobs.php
Sending        webapp-php/application/models/report.php
Adding         webapp-php/application/views/common/error.php
Sending        webapp-php/application/views/report/index.php
Sending        webapp-php/application/views/report/pending.php
Transmitting file data ...........
Committed revision 2360.

Comment 15

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

Removing 2 methods from the Report Model that were no longer necessary.

==

Sending        models/report.php
Transmitting file data .
Committed revision 2428.