Closed Bug 1135700 Opened 6 years ago Closed 6 years ago

Switch "make uploadsymbols" to use Socorro Symbol Upload API

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox39 fixed, firefox-esr31 fixed, firefox-esr38 fixed, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.1S fixed, b2g-v2.2 fixed, b2g-master fixed)

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed
firefox-esr31 --- fixed
firefox-esr38 --- fixed
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: ted, Assigned: ted)

References

Details

Attachments

(2 files, 1 obsolete file)

We're going to switch everything that's uploading symbols to using the Socorro symbol upload API (see the bug this blocks and the dependency tree). Socorro has a simple REST API for uploading symbols, so this should not be a lot of code.

Testing this is kind of a pain because it's Nightly/Release only, and a) we don't have a way to test nightlies on try, and b) even if we did the try pool is separate from the build pool so they wouldn't have the right authentication token available. The current plan of action is to add this upload method alongside the existing SSH upload method, initially pointed at the Socorro staging server to shake out any issues. After that looks okay we'll point it at production but still run both upload methods for a short time (we're syncing the NFS mount to S3 so there will be duplication but it's not a problem on Socorro's end). Once we're comfortable that it's working we'll remove the old SSH upload method.
Attached file MozReview Request: bz://1135700/ted (obsolete) —
/r/4187 - bug 1135700 - Import the Python requests module
/r/4189 - bug 1135700 - make uploadsymbols use Socorro symbol upload API

Pull down these commits:

hg pull review -r 0f152208e9951157623ff57dbe00573ebef7ebff
Attachment #8568101 - Flags: review?(gps)
I'm importing the requests module because it makes the actual HTTP upload here a 1-liner.
https://reviewboard.mozilla.org/r/4187/#review3335

A decent HTTP API in the tree \o/. I can actually make use of this in bug 1132771, since I would like to write a mach command that audits Bugzilla compoonents and code reviews against what's actually configured on bugzilla.mozilla.org!
https://reviewboard.mozilla.org/r/4189/#review3337

While I raised a few issues, none are significant enough to block landing.

::: toolkit/crashreporter/tools/upload_symbols.py
(Diff revision 1)
> +#

Please add a quick comment to this file indicating its purpose.

::: toolkit/crashreporter/tools/upload_symbols.py
(Diff revision 1)
> +    token_file = substs['SOCORRO_SYMBOL_UPLOAD_TOKEN_FILE']

I hate the buildconfig module with passion. If you could pass in the path to the toke file via CLI args, I'd like this patch even better.

::: toolkit/crashreporter/tools/upload_symbols.py
(Diff revision 1)
> +    r = requests.post(
> +        url,
> +        files={'symbols.zip': open(sys.argv[1], 'rb')},
> +        headers={'Auth-Token': auth_token},
> +        allow_redirects=False
> +    )

Should we add an explicit timeout?

Also, consider throwing this behind a try..except requests.exceptions.RequestException.

::: toolkit/crashreporter/tools/upload_symbols.py
(Diff revision 1)
> +        files={'symbols.zip': open(sys.argv[1], 'rb')},

Using files= will use multi-part mime with base64 encoding, no? This will result in significant encoding overhead for symbols.zip. This will consume a lot of excessive bandwidth.

I don't suppose there's a way to transfer the data in raw/binary form to avoid the overhead? (I understand this may be out of your control and a follow-up bug.)
Comment on attachment 8568101 [details]
MozReview Request: bz://1135700/ted

https://reviewboard.mozilla.org/r/4185/#review3339

Ship It!
Attachment #8568101 - Flags: review?(gps) → review+
https://reviewboard.mozilla.org/r/4185/#review3341

I forgot to mention that your need to import `requests` is debatable. But I like having access to powerful libraries, so I'll allow it.
https://reviewboard.mozilla.org/r/4189/#review3351

> I hate the buildconfig module with passion. If you could pass in the path to the toke file via CLI args, I'd like this patch even better.

Any particular reason? For a single-purpose script I feel like having the knowledge of what files it needs baked in makes more sense than having to pass them as parameters. (Also it lets me easily write useful error messages here vs. having to do it in make.)

> Using files= will use multi-part mime with base64 encoding, no? This will result in significant encoding overhead for symbols.zip. This will consume a lot of excessive bandwidth.
> 
> I don't suppose there's a way to transfer the data in raw/binary form to avoid the overhead? (I understand this may be out of your control and a follow-up bug.)

The API accepts the file as a binary blob, but I'd also like to be able to send metadata along with it (which it doesn't currently accept) which would necessitate using multipart/form-data anyway.
https://reviewboard.mozilla.org/r/4189/#review3457

> Any particular reason? For a single-purpose script I feel like having the knowledge of what files it needs baked in makes more sense than having to pass them as parameters. (Also it lets me easily write useful error messages here vs. having to do it in make.)

mozbuild.base.MozbuildObject.from_environment() is preferred over buildconfig. buildconfig is a holdover from before we had nice APIs. It does get the job done in an easy-to-use way, I concede.

Don't worry about fixing this if you don't want to.

> The API accepts the file as a binary blob, but I'd also like to be able to send metadata along with it (which it doesn't currently accept) which would necessitate using multipart/form-data anyway.

I just wanted to raise the concern. As long as it is known to you and you are fine with that, it's ultimately your choice to use the inefficient encoding. Feel free to drop the issue.
https://reviewboard.mozilla.org/r/4189/#review3577

> Should we add an explicit timeout?
> 
> Also, consider throwing this behind a try..except requests.exceptions.RequestException.

Both good suggestions. I'm putting a 60 second timeout in there for now, the requests documentation says the timeout isn't global for the request, but only "have waited this long without a response from the server". We'll have to see if that's a problem in production, since the server side is going to be synchronously uploading the contents of the zip to S3.
Attachment #8568101 - Attachment is obsolete: true
Attachment #8619561 - Flags: review+
Attachment #8619562 - Flags: review+
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.