636085 - generate_update_statistics should only report Sparkle pings

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I thought we were already doing this, but I guess not; tonight I noticed (because they had broken syntax which affected the 2.0 l10n columns) pings from some Verizon FIOS customer in DC using Camino itself with bogus OS versions and &-escaped &s in the query string.

And, indeed, on the same day (19 Feb), there were 693 pings that didn't have Sparkle in the UA.

Comment 1

[Samuel Sidler (:ss)]

This should be as easy as adding...

  grep "Sparkle"

... just before or after each of the...

  grep " /update-check?os="

... lines throughout generate_update_statistics (with the appropriate pipe). I haven't tested it, but there's no reason it should be any harder than that.

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Yeah, I planned on using "Sparkle/".  I filed this mostly because I didn't have time to really mess with it that night, and also because I wondered if there were a way to do it without triggering Yet Another Grep Invocation™ ;)

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Exclude non-sparkle pings

I opted to put it just before the grep " /update-check?os=" check, for two reasons:

1) It keeps all of the "used-everywhere" parts of the code in the same place/order in every check, and 
2) It keeps both "let's exclude stuff" checks (badIPs and Sparkle) adjacent to each other.

I spot-checked Feb 21 and the difference between the total pings generated earlier in the week and the total pings generated with my changes are exactly the same as the number of pings matching all the other criteria but missing Sparkle (i.e., using grep -v "Sparkle/").

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #2)
> were a way to do it without triggering Yet Another Grep Invocation™ ;)

Ideally, I think, we'd want to factor out the 

grep "$date" $file | grep -Ev "$badips" | grep "Sparkle/"

and 

grep "$date" $file | grep -Ev "$badips" | grep "Sparkle/" | grep " /update-check?os="

things that are common and used all over and only run those once, rather than on every stat.  I haven't figured out how to do that, though (and it's really another bug :P ).

Comment 5

[Stuart Morgan]

(In reply to comment #4)
> I haven't figured out how to do that, though

Convert this to a Real Language, instead of bash? ;)

I can try to rubyify this at some point if you like.

Comment 6

[Samuel Sidler (:ss)]

(In reply to comment #4)
> things that are common and used all over and only run those once, rather than
> on every stat.  I haven't figured out how to do that, though (and it's really
> another bug :P ).

As Stuart says, we can convert it to a Real Language like Python (see what I did there?), but in the mean time, we can make the things you want to factor out into one big variable (e.g. $everything and $everything_with_update-check) and simply define it after the other variables. Messy and ugly, but it'll save time when we have updates like this in the future (though... it's rare enough and find/replace works well).

Comment 7

[Samuel Sidler (:ss)]

Comment on attachment 515177 [details] [diff] [review]
Exclude non-sparkle pings

r=me

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://hg.mozilla.org/camino/rev/d52651dc5f21

(In reply to comment #6)
> did there?), but in the mean time, we can make the things you want to factor
> out into one big variable (e.g. $everything and $everything_with_update-check)
> and simply define it after the other variables.

That doesn't actually work; I tried that before making comment 4 ;)

Filed bug 637091 on the Real Language stuff ;-)