648068 - In Blocklist, Remove the OOB value "reset" from days since last request

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Daniel Einspanjer [:dre] [:deinspanjer]]

I have looked through the code and as near as I can tell, "reset" should only show up if the "ping count" is a negative number.  The only place in the code where ping count can be set to a negative number is if it rolls over.  Neither of these scenarios makes a lot of sense to me, but we are now getting millions of requests with the "reset" value per day and I have no confidence in my understanding of what that means or how to treat them.

Comment 1

[Daniel Einspanjer [:dre] [:deinspanjer]]

Okay, I found the cause in AddonManager.jsm.  When the appChanged flag is set to true, the ping count variable will be -1 which will cause days since last to be "reset".

Changing this bug to request the removal of this special value.  It forces me to undercount when a browser is upgraded as the first event in the time period being counted.

If we instead just returned a 0 or 1 or whatever the proper value is, the upgrade would not impact the metric at all.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I would say it should block a release on 2.0. Otherwise we can't get the correct metrics. Requesting for blocking.

Comment 3

[Dave Townsend [:mossop]]

Reading through bug 616835 it seems that this is doing exactly what was requested, am I misreading things or is this just a request to change to a different behaviour?

Comment 4

[Daniel Einspanjer [:dre] [:deinspanjer]]

Yes, this is a request to change to a different behavior.  When this method was prototyped, the reset value was useful to be able to look at the upgrade patterns.  After analyzing several weeks of data from the 4.0 release though we have found that it creates a hole in the metric.  

I don't intend to be making any more feature requests to the blocklist metrics because we are working on our proposal for moving the mechanism to a separate service, but this is the removal of a line of code that will improve these existing metrics, so I am hoping we can work it in to the existing pipeline.

The data hole is noticeable but small so I didn't want to try to raise it as an immediate blocker, but since our new service isn't going to be in Firefox 5 it would be good to be fixed before then.

Comment 5

[Dave Townsend [:mossop]]

Ok, sounds like it doesn't make sense to try to block 4.0 on this, we'll try to find someone to get it done in time for Firefox 5.

Comment 6

[Daniel Einspanjer [:dre] [:deinspanjer]]

Attachment: Patch to remove "reset" value with additional unit tests

Would it help the possibility of getting it into 4.0.1 if I can put together an acceptable patch myself or would it be disruptive to other work that is better suited to be considered blocking?

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 524357 [details] [diff] [review]
Patch to remove "reset" value with additional unit tests

Certainly, if we can get this landed we can request approval, but I know the branch drivers are trying to take only critical fixes for 4.0.1 so you'll need to elaborate on why it is such an important fix to take.

This patch seems ok but it is failing tests with:

TEST-UNEXPECTED-FAIL | /Users/dave/mozilla/build/minefield/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell-unpack/test_bug620837.js | 1&3&2 == 1&3&1

Comment 8

[Daniel Einspanjer [:dre] [:deinspanjer]]

Okay, I was having trouble getting the test set up to run on my sandbox so I eyeballed them.  Let me review the order of operations on that one and figure out what I did wrong.  Will attach a new patch shortly.

Comment 9

[Daniel Einspanjer [:dre] [:deinspanjer]]

Attachment: Patch to remove "reset" value with additional unit tests

Okay, test4 should have a days since value of 2 since the last ping time was set to two days ago.  Typo on my part.

Comment 10

[Daniel Einspanjer [:dre] [:deinspanjer]]

The justification for taking the fix in 4.0.1 is that it gives us a chance to close this data hole before 4.0 is pushed out to our largest pool of users.  While the hole isn't destructive, it is definitely better to close quickly than waiting.

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 524422 [details] [diff] [review]
Patch to remove "reset" value with additional unit tests

Looks good

Comment 12

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/de57e98c7a28

Comment 13

[Daniel Einspanjer [:dre] [:deinspanjer]]

Who should I talk to to explore the possibility of getting this into 4.0.1?

Comment 14

[Robert Strong (they/them - no direct email)]

View the details of the patch and request approval2.0=? for the patch. The drivers triage the approval2.0 queue periodically.

Comment 15

[Robert Strong (they/them - no direct email)]

Also include reasons why the patch should be accepted.

Comment 16

[Daniel Einspanjer [:dre] [:deinspanjer]]

Comment on attachment 524422 [details] [diff] [review]
Patch to remove "reset" value with additional unit tests

please see comment #10 for possible justification.

Comment 17

[Daniel Einspanjer [:dre] [:deinspanjer]]

So is this patch still eligible for inclusion in Macaw?

Comment 18

[Dave Townsend [:mossop]]

We have to wait for branch drivers to triage the approval requests.

Comment 19

[Daniel Einspanjer [:dre] [:deinspanjer]]

The patch is marked as blocking2.0? but the bug isn't showing up in a query someone mentioned on the release-drivers list which makes me wonder if it could be overlooked.

Comment 20

[Daniel Einspanjer [:dre] [:deinspanjer]]

Oh, it is in a query for approval2.0, don't know how that relates to blocking2.0.  I'll put this back the way I found it and just hope for the best and prepare for the worst. :)

Comment 21

[Robert Strong (they/them - no direct email)]

This made it in to mozilla-central before the creation of mozilla-aurora so this is already in for Firefox 5.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #20)
> Oh, it is in a query for approval2.0, don't know how that relates to
> blocking2.0.  I'll put this back the way I found it and just hope for the
> best and prepare for the worst. :)

I don't think that this patch will get approval for 2.0. So we should probably remove the request now and wait for Firefox 5.

Comment 23

[Daniel Einspanjer [:dre] [:deinspanjer]]

Just to make sure that I am clear on the status of this critical bug fix:
The fix is in 5.0 and has been since before 5.0 went beta.  It is not in 4.0.1 and we don't have any intention of releasing a 4.0.2 at this point, correct?

Comment 24

[Dave Townsend [:mossop]]

Correct