965587 - Prevent client from inadvertently submitting two records at the same time

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[Richard Newman [:rnewman]]

We've seen behaviors like this:

---
 A = 1d0af
 B = 708235
 C = 4801
 1. PUT A, DELETE B,C
 2. PUT C, DELETE B,A

The second is

 A = 480ee
 B = f587
 C = 73e6
 1. PUT A, DELETE B,C
 2. PUT C, DELETE B,A
---

This can only be explained by a timer firing twice on a client, such that it tries to do two uploads at the same time.

The trivial solution for this is to not allow the client to make two requests at the same time.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Add upload lock

rnewman granted review in RB.

Comment 2

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb793d3d2bad

Comment 3

[Gregory Szorc [:gps]]

I'm going to tentatively flag this for tracking 28, as it has the potential to reduce orphaned record production in the release channel 6 weeks sooner and is a relatively simple patch.

We should wait on some numbers from Nightly and Aurora to assess the impact of this patch before landing.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bb793d3d2bad

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

Not blocking release, but if you want to nominate a low-risk uplift to an early FF28 beta please go ahead.

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 8367965 [details]
Add upload lock

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Firefox Health Report
User impact if declined: Possibly making server orphans worse
Testing completed (on m-c, etc.): Tests with patch. Still waiting on confirmation from Metrics
Risk to taking this patch (and alternatives if risky): It's touching the FHR code upload path. Worst case is it screws up upload. Chance of that happening should be low.
String or IDL/UUID changes made by this patch: None

This is a small patch and should have no adverse impact. If we didn't have the ability to measure the impact of this patch via FHR record submissions, I'd say uplift with few questions asked. But we do have the ability to measure it. I'm requesting approval now on the condition that Metrics says this doesn't appear to negatively impact FHR uploads on Nightly and/or Aurora channels.

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8367965 [details]
Add upload lock

Alright then, let's get this in early and capture that data.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e78826467135

Comment 9

[Gregory Szorc [:gps]]

jjensen: Can you assign someone to look at the FHR submission counts for Beta 28 starting in a week or so? We'd like to verify submission counts don't fall off a cliff before this patch rides to Release.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/e78826467135

Comment 11

[Gregory Szorc [:gps]]

Why was this uplifted to b2g28? FHR isn't active there. No harm, no foul. Just curious.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://wiki.mozilla.org/Release_Management/B2G_Landing#Landing_Procedure_4

Comment 13

[u279076]

Assuming verification my Metrics that this is working correctly is verification enough. Please let me know if QA is needed here.