461089 - Purpose of appVersion variable changing in Firefox 3.1

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Statistics, defect)

Description

[Justin Scott [:fligtar]]

With Firefox 3.1 and bug 324121, the VersionCheck parameter is being redefined as the version of the application for which compatibility is being checked, which is not necessarily the same as the currently running version.

I've asked that we bump the reqVersion for this change, so we need to add logic to the stats parser where if reqVersion is 2, we use the new currentAppVersion variable, and if it's not 2 then we use appVersion for the version of the application being used.

We should do this soon, as it sounds like they are waiting to land the pref change until this happens.

Comment 1

[Michael Morgan [:morgamic]]

Fligtar, if you have time feel free to take this otherwise I need you to review it.

Comment 2

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

Bug 324121 will likely land sometime in the next couple of days. It has string changes so I'd like to know whether I can land prior to this bug being fixed.

Comment 3

[Justin Scott [:fligtar]]

You can land the client changes whenever you need to. It's not going to have a significant enough effect on stats until long after we've fixed this bug, which will be in the upcoming AMO release.

Comment 4

[Justin Scott [:fligtar]]

Attachment: patch, v1

Patch to determine the correct variable to use for the current client version.

I still need to test this.

Comment 5

[Justin Scott [:fligtar]]

Attachment: patch, v2

Updated patch since clouserw's collection stats patch changed a few things.

Comment 6

[Justin Scott [:fligtar]]

Attachment: patch, v3

This patch fixes a regex problem discovered while testing.

I tested using a test logfile I'll attach soon, and this patch worked as expected.

Comment 7

[Justin Scott [:fligtar]]

Attachment: test logfile

Instructions to test patch:
1) Save attached test logfile to a directory
2) Run the following after patch applied:
php -f parse_logs/parse_logs.php logs=/path/to/fake/log/dir temp=/tmp type=updatepings geo=SJ date=2008-12-07 v

3) Run this sql
mysql -u root remora -e "select application from update_counts where date='2008-11-25' and addon_id=4938"

The result should be a serialized array with 2 different Firefox versions: 2.0.0.18 and 3.1. This is because the first log line was a reqVersion 1 request using the old format, and the second line is a reqVersion 2 request with currentAppVersion. If the patch was not applied, the 2 versions listed would be 2.0.0.18 and 3.1.1

Comment 8

[Michael Morgan [:morgamic]]

Comment on attachment 351743 [details] [diff] [review]
patch, v3

This looks good but we should write tests for this.  Wil wrote tests for everything in bug 462814 and it should be pretty easy to add the stuff from comment #7.  Since this is stats we should just write the tests.

Comment 9

[Justin Scott [:fligtar]]

Since we'll be switching off of this AMO stats script this week (hopefully), there's not much point in resolving this now. We just need to verify that the scripts we'll be switching to handle this properly.

Daniel, do your add-on update ping counting scripts get the client application version info from appVersion in < Firefox 3.1 and from currentAppVersion in >= Firefox 3.1?

Comment 10

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

No, I was not notified of any changes to the versioncheck API (another endorsement for my idea of metrics specific tests listed in bug 469806).
I'm going to have to write a new module to handle this new format.

What exactly does "parameter is being redefined as the version of the application for which compatibility is being checked" mean?  Could you walk me through a scenario for which an extension is pinging vc for an application version that is different from the currently running version?

Comment 11

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

Daniel, I just sent you the email thread regarding this which you were cc'd on.

The appVersion was always meant to be used for the version of the app the update check was being made for. The code always sent appVersion as the currently running version until recently when it was made to behave as it was originally designed to behave which then meant the value could be the version the user is upgrading to. In order to accomplish this and still support the existing use of appVersion by metrics currentAppVersion was added to the url.

Comment 12

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

Yep. I was completely mistaken about not being notified.  I was CC'ed on a thread back in September about this change.  Unfortunately, development of the Metrics processor for VersionCheck was not truely started until november, and this change was not properly handled by me.

I'll make sure that gets taken care of after we get the most pressing AMO work finished.  I've been reading through other bugs related to this change, and it sounds like the use case I was asking for earlier would be as follows, please let me know if I have it right:

If the user is on Firefox 3.1.0 with extension A version 1.2 installed, and they are going about their normal business, once a day, there will be a VC ping from extension A checking to see if there is an update available with appVersion = 3.1.0 and currentAppVersion = 3.1.0.  

If on the next day, the user receives notification that a new version of Firefox is available, 3.1.1, at some point during the upgrade process, the following will happen:
A VC ping will go out for extension A checking to see if there is an update available with appVersion = 3.1.1 and currentAppVersion = 3.1.0.  This is so that the update process can determine whether it should list extension A as an extension that will be disabled for compatibility reasons after the update.

Is that correct?

Comment 13

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

That is correct for the most part except that there will only be a VC ping for extension A if extension A is not compatible with the new version of the App which in your example would be 3.1.1.

Comment 14

[Wil Clouser [:clouserw]]

This is a WONTFIX since we're switching to Daniel's script.

Comment 15

[Wil Clouser [:clouserw]]

Since everyone is CC'd on this bug already I'll just reopen and reassign to Daniel.

Comment 16

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

Change is implemented in new AMO ETL process.  Push to happen this weekend 2009-06-06