1609440 - antenna should distinguish between rejected because of rule and rejected because of malformed

Status: RESOLVED FIXED

(Socorro :: Antenna, enhancement, P2)

Description

[Will Kahn-Greene [:willkg] ET needinfo? me]

Antenna can reject incoming crash reports for two major reasons:

1. because a throttler rule told it to reject the crash report
2. because the crash report is malformed

In the former case, the crash reporter client may submit the crash report again. In the latter case, the crash reporter client has a bug and that should get fixed.

Currently, it's tough to distinguish between the two cases.

This bug covers fixing the malformed case to make it clearer that the rejection reason is because the crash report is malformed.

Comment 1

[Will Kahn-Greene [:willkg] ET needinfo? me]

Making this a P2 to do soon. It'll aid in debugging crash reporter client problems.

Comment 2

[Will Kahn-Greene [:willkg] ET needinfo? me]

I'm thinking of doing two things:

1. changing the HTTP status code of the response for malformed crash reports to HTTP 400
2. changing the HTTP body to Discarded=malformed_REASON

I know we had a problem with B2G where it didn't handle rejection--it would keep retrying. That was bad and we had to add "fake accept" code to the collector.

I think if the crash report is malformed, it's because there's a bug in the crash reporter client and we'd want to surface bugs sooner and an HTTP 400 might help with that.

Gabriele: Do you think these changes will be safe for crash reporter clients?

Comment 3

[Will Kahn-Greene [:willkg] ET needinfo? me]

Attachment: pr 421: bug 1609440: distinguish between rejections

Comment 4

[Will Kahn-Greene [:willkg] ET needinfo? me]

In Slack, Gabriele said this:

we've got three crash reporters to address, the in-Firefox crash reporter for content/plugin crashes, the crash reporter client for browser crashes and the GeckoView/Fenix one. It's likely that the first can handle failures but I don't know about the others. We may also have to check out Fennec ESR since it's going to be around for some time still.

We had 100 malformed crash reports in the last week, so I can make this change, push it out, keep an eye on it, and we'll probably be ok.

Comment 5

[Will Kahn-Greene [:willkg] ET needinfo? me]

willkg merged PR #421: "bug 1609440: distinguish between rejections" in c7c0c91.

Comment 6

[Gabriele Svelto [:gsvelto]]

BTW I just checked and the fake server (crashreport.sjs) we use in tests returns HTTP 400 for malformed requests. I didn't check all the tests but there's hope that we're handling that stuff correctly.

Comment 7

[Will Kahn-Greene [:willkg] ET needinfo? me]

Deployed just now as part of bug #1609747. I added a graph to datadog showing 5xx and 4xx from the collector ELB. So far there aren't any 4xx. I'll keep tabs on it for the next week, but everything looks good so far.

Marking as FIXED.