802734 - Add pings for metrics to the stub installer

Status: VERIFIED FIXED

(Firefox :: Installer, defect)

Description

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

I'll add more details later

Comment 1

[Anurag Phadke[:[e-mail]]]

We can have a simple HTTP request sent to www.mozilla.org (or any domain) which returns a 200 code with blank page. metrics has access to logs and can parse these results.
:rstrong - feel free to assign me this bug once u fill in the details.

Comment 2

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

(In reply to aphadke from comment #1)
> We can have a simple HTTP request sent to www.mozilla.org (or any domain)
> which returns a 200 code with blank page. metrics has access to logs and can
> parse these results.
Can I get a base url that you would like me to use?

> :rstrong - feel free to assign me this bug once u fill in the details.
This bug will be used to add the code to the stub installer itself.

Comment 3

[Anurag Phadke[:[e-mail]]]

(deferring to chris more as he owns www.mozilla.org)

:chrismore - can we add a page to mozilla.org to collect the following pings from stub-installer.

base-url: http://www.mozilla.org/stub/status.php
each ping would be passed on as an argument.

for eg:
http://www.mozilla.org/stub/status.php?start_download=1
http://www.mozilla.org/stub/status.php?finish_download=1
http://www.mozilla.org/stub/status.php?start_install=1
http://www.mozilla.org/stub/status.php?end_install=1
http://www.mozilla.org/stub/status.php?start_exe=1

/stub/status.php will be an empty page returning a 200 request. The goal out here is to collect this information in zeus logs and then understand the drop-off.

Comment 4

[Chris More [:cmore]]

Is there a reason why this has to be on mozilla.org as opposed to another metrics collector server? I think having a status ping is great!

We should loop in privacy (Stacy Martin) asap so she is aware that we are looking to track something initiated by a user. As long as the web server that is be hit by this ping is following our normal log policies (no PII), we should be fine.

Just to clarify, you will just be doing log analysis with the query strings and no database is installed, right?

The status page can be really a blank HTML.

Comment 5

[Anurag Phadke[:[e-mail]]]

(adding daniel and xavier to shed light on collecting data on metrics servers)

Comment 6

[Jason Smith [:jsmith]]

Is this a stop ship bug for the english deployment of the stub installer? If it is, can someone tack on the [stub+] whiteboard tag?

Comment 7

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

It is for the expedited rev2 to evaluate the stub that we discussed on Tuesday.

Comment 8

[Anurag Phadke[:[e-mail]]]

:rstrong - should we have a 30-meeting with the folks on this bug and Stacy to discuss the pings, collection server and privacy?

Comment 9

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

aphadke, I don't have much time available with everything else going on so please just be patient until I have a chance to post the info you are asking for to this bug. Thanks!

Comment 10

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

Alternatively, you could have a meeting, document the data points you'd like, and I'll merge it with the existing data points and remove the ones that aren't possible. Keep in mind that it also might not be possible to implement all of the data points on the first implementation.

Comment 11

[Anurag Phadke[:[e-mail]]]

Alright, lemme schedule a meeting, last thing we want is for u to be code ready and we not having the servers ready or making the privacy team nervous on what we are measuring.

Comment 12

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

That would be excellent if you and the metrics team could take that part on so I can focus on the coding and other work. Thanks!

Comment 13

[Tom Lowenthal]

Oh hey, I think I should take privacy point on this rather than Stacy. Thanks!

Comment 14

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

Attachment: patch rev1

This doesn't get us the world but this does get us what we need for the time being.

Comment 15

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

Brian, please ignore the url used. I'll get the final url that will be used later.

Comment 16

[Brian R. Bondy [:bbondy]]

Comment on attachment 674630 [details] [diff] [review]
patch rev1

Review of attachment 674630 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/installer/windows/nsis/stub.nsi
@@ +82,5 @@
> +!define ERR_CERT_UNTRUSTED 12
> +!define ERR_CERT_ATTRIBUTES 13
> +!define ERR_CERT_UNTRUSTED_AND_ATTRIBUTES 14
> +!define ERR_CHECK_INSTALL_TIMEOUT 15
> +!define ERR_UNKNOWN 99

Maybe we should add another value for .onUserAbort?

@@ +350,5 @@
> +    Call GetSecondsToDownload
> +  ${EndIf}
> +
> +  ; Try to send a ping if a download was attempted
> +  ${If} $IsDownloadFinished != ""

Wouldn't it be useful data to know the difference between the people who didn't download but did open the stub?  You'd have to default the other values to some invalid value in that case.  In that case you'd also be able to collect extra data like 1) if the installer didn't run because of min supported OS, 2) if there was not enough space to start the installer, ... possibly others.

@@ +351,5 @@
> +  ${EndIf}
> +
> +  ; Try to send a ping if a download was attempted
> +  ${If} $IsDownloadFinished != ""
> +    System::Int64Op $DownloadedAmount / 1024

You want this reported in KiB right? I mean as opposed to just using bytes directly.

@@ +359,5 @@
> +    ; Server url that the stub downloaded the install from
> +    ; Windows version including service pack
> +    ; 32 or 64 bit OS
> +    ; Install in default location
> +    InetBgDL::Get "${BaseURLStubPing}v1/$ExitCode/$FirefoxLaunch/" \

As mentioned I'll ignore this for now.

@@ +361,5 @@
> +    ; 32 or 64 bit OS
> +    ; Install in default location
> +    InetBgDL::Get "${BaseURLStubPing}v1/$ExitCode/$FirefoxLaunch/" \
> +                  "$SecondsToDownload/$DownloadedAmount/$ExistingProfile/" \
> +                  "$ExistingInstall/${AB_CD}/${Channel}" "$PLUGINSDIR\_temp" /END

how about other data like the value of the checkbox prefs?  I'm guessing these were omitted on purpose for privacy concerns.

@@ +864,5 @@
> +  ${Else}
> +    StrCpy $ExistingInstall 0
> +  ${EndIf}
> +
> +  ${If} ${FileExists} "$LOCALAPPDATA\Mozilla\Firefox"

We should have \brand here as well.

@@ +894,5 @@
> +Function GetSecondsToDownload
> +  GetTempFileName $0
> +  GetFileTime $0 $1 $2
> +  Delete $0
> +  System::Int64Op $2 - $SecondsToDownload

This won't always push the seconds to download onto the stack.  If the high DWORD changes between the 2 calls, then this will return a negative value.  It won't happen very often, but just calling it out.

@@ +1012,5 @@
> +            StrCpy $ExitCode ${ERR_CERT_UNTRUSTED}
> +          ${ElseIf}  $1 == 0
> +            StrCpy $ExitCode ${ERR_CERT_ATTRIBUTES}
> +          ${EndIf}
> +        ${EndIf}

I think this whole block would be cleaner outside of the containing
${If} $0 == 0
${OrIf} $1 == 0

And it would be safe to do so before the containing ifs.

Comment 17

[Brian R. Bondy [:bbondy]]

No major concerns, mostly just questions and "should we do X to get data of type Y?" type questions.

Comment 18

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

(In reply to Brian R. Bondy [:bbondy] from comment #16)
> Comment on attachment 674630 [details] [diff] [review]
> patch rev1
> 
> Review of attachment 674630 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/installer/windows/nsis/stub.nsi
> @@ +82,5 @@
> > +!define ERR_CERT_UNTRUSTED 12
> > +!define ERR_CERT_ATTRIBUTES 13
> > +!define ERR_CERT_UNTRUSTED_AND_ATTRIBUTES 14
> > +!define ERR_CHECK_INSTALL_TIMEOUT 15
> > +!define ERR_UNKNOWN 99
> 
> Maybe we should add another value for .onUserAbort?
See below

> 
> @@ +350,5 @@
> > +    Call GetSecondsToDownload
> > +  ${EndIf}
> > +
> > +  ; Try to send a ping if a download was attempted
> > +  ${If} $IsDownloadFinished != ""
> 
> Wouldn't it be useful data to know the difference between the people who
> didn't download but did open the stub?  You'd have to default the other
> values to some invalid value in that case.  In that case you'd also be able
> to collect extra data like 1) if the installer didn't run because of min
> supported OS, 2) if there was not enough space to start the installer, ...
> possibly others.
If we report cases where the download hasn't started then we will prompt users with Firewalls to allow the stub to make an internet connection which I think would be a very bad user experience (e.g. this thing is trying to make an internet connection when all I am doing is cancelling it). I'm not saying we won't do this at some point but I do want us to consider the ramifications of doing this more thoroughly before making the decision to do this.

> @@ +351,5 @@
> > +  ${EndIf}
> > +
> > +  ; Try to send a ping if a download was attempted
> > +  ${If} $IsDownloadFinished != ""
> > +    System::Int64Op $DownloadedAmount / 1024
> 
> You want this reported in KiB right? I mean as opposed to just using bytes
> directly.
Yes

> 
> @@ +359,5 @@
> > +    ; Server url that the stub downloaded the install from
> > +    ; Windows version including service pack
> > +    ; 32 or 64 bit OS
> > +    ; Install in default location
> > +    InetBgDL::Get "${BaseURLStubPing}v1/$ExitCode/$FirefoxLaunch/" \
> 
> As mentioned I'll ignore this for now.
Not sure what you are referring to

> 
> @@ +361,5 @@
> > +    ; 32 or 64 bit OS
> > +    ; Install in default location
> > +    InetBgDL::Get "${BaseURLStubPing}v1/$ExitCode/$FirefoxLaunch/" \
> > +                  "$SecondsToDownload/$DownloadedAmount/$ExistingProfile/" \
> > +                  "$ExistingInstall/${AB_CD}/${Channel}" "$PLUGINSDIR\_temp" /END
> 
> how about other data like the value of the checkbox prefs?  I'm guessing
> these were omitted on purpose for privacy concerns.
I can see value in knowing whether any shortcuts were installed and the maintenance service being info that we'd like to know. Not concerned about it for the purposes of this bug though.

> 
> @@ +864,5 @@
> > +  ${Else}
> > +    StrCpy $ExistingInstall 0
> > +  ${EndIf}
> > +
> > +  ${If} ${FileExists} "$LOCALAPPDATA\Mozilla\Firefox"
> 
> We should have \brand here as well.
Why? The profiles are always under Firefox for all channels
%LOCALAPPDATA%\Mozilla\Firefox

> 
> @@ +894,5 @@
> > +Function GetSecondsToDownload
> > +  GetTempFileName $0
> > +  GetFileTime $0 $1 $2
> > +  Delete $0
> > +  System::Int64Op $2 - $SecondsToDownload
> 
> This won't always push the seconds to download onto the stack.  If the high
> DWORD changes between the 2 calls, then this will return a negative value. 
> It won't happen very often, but just calling it out.
I've been sick this week and would appreciate some details as to why so I don't have to exercise my NyQuil addled brain cells.

> 
> @@ +1012,5 @@
> > +            StrCpy $ExitCode ${ERR_CERT_UNTRUSTED}
> > +          ${ElseIf}  $1 == 0
> > +            StrCpy $ExitCode ${ERR_CERT_ATTRIBUTES}
> > +          ${EndIf}
> > +        ${EndIf}
> 
> I think this whole block would be cleaner outside of the containing
> ${If} $0 == 0
> ${OrIf} $1 == 0
> 
> And it would be safe to do so before the containing ifs.
ok

Comment 19

[Brian R. Bondy [:bbondy]]

> > @@ +359,5 @@
> > > +    ; Server url that the stub downloaded the install from
> > > +    ; Windows version including service pack
> > > +    ; 32 or 64 bit OS
> > > +    ; Install in default location
> > > +    InetBgDL::Get "${BaseURLStubPing}v1/$ExitCode/$FirefoxLaunch/" \
> > 
> > As mentioned I'll ignore this for now.
> Not sure what you are referring to

I'm referring to the TODO comment and Comment 15.


> > This won't always push the seconds to download onto the stack.  If the high
> > DWORD changes between the 2 calls, then this will return a negative value. 
> > It won't happen very often, but just calling it out.
> I've been sick this week and would appreciate some details as to why so I
> don't have to exercise my NyQuil addled brain cells.

GetFileTime returns a high order DWORD and a low order DWORD.  Both make up a part of the same number.  You're ignoring the high order DWORD.  So sometimes the low order DWORD will near max out and then the next call will go back to 0 and the high order DWORD will increase.  So in that case the first time you call GetFileTime it will look like a later time than the second. 

> > We should have \brand here as well.
> Why? The profiles are always under Firefox for all channels
> %LOCALAPPDATA%\Mozilla\Firefox

Because otherwise we'd be reporting incorrectly.  For example it would look like almost all Nightly installs are re-installs. But in reality it's just that these people typically have both Firefox and Nightly installed. But it is a first time Nightly install.

Comment 20

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

(In reply to Brian R. Bondy [:bbondy] from comment #19)
> > > @@ +359,5 @@
> > > > +    ; Server url that the stub downloaded the install from
> > > > +    ; Windows version including service pack
> > > > +    ; 32 or 64 bit OS
> > > > +    ; Install in default location
> > > > +    InetBgDL::Get "${BaseURLStubPing}v1/$ExitCode/$FirefoxLaunch/" \
> > > 
> > > As mentioned I'll ignore this for now.
> > Not sure what you are referring to
> 
> I'm referring to the TODO comment and Comment 15.
> 
> 
> > > This won't always push the seconds to download onto the stack.  If the high
> > > DWORD changes between the 2 calls, then this will return a negative value. 
> > > It won't happen very often, but just calling it out.
> > I've been sick this week and would appreciate some details as to why so I
> > don't have to exercise my NyQuil addled brain cells.
> 
> GetFileTime returns a high order DWORD and a low order DWORD.  Both make up
> a part of the same number.  You're ignoring the high order DWORD.  So
> sometimes the low order DWORD will near max out and then the next call will
> go back to 0 and the high order DWORD will increase.  So in that case the
> first time you call GetFileTime it will look like a later time than the
> second. 
Thanks and yes, my brain is rather addled right now.

> > > We should have \brand here as well.
> > Why? The profiles are always under Firefox for all channels
> > %LOCALAPPDATA%\Mozilla\Firefox
> 
> Because otherwise we'd be reporting incorrectly.  For example it would look
> like almost all Nightly installs are re-installs. But in reality it's just
> that these people typically have both Firefox and Nightly installed. But it
> is a first time Nightly install.
That isn't for determining reinstalls... just that there was a profile created at some point (e.g. system with / without profiles is what this tells us). Also, brand wouldn't work here since it isn't part of the filesystem path for the profile. I think you might be seeing "UpdRootD" dir that is used when installed under Program Files but isn't required.

Comment 21

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

isn't required to match brand that is

Comment 22

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

(In reply to Brian R. Bondy [:bbondy] from comment #19)
> > > @@ +359,5 @@
> > > > +    ; Server url that the stub downloaded the install from
> > > > +    ; Windows version including service pack
> > > > +    ; 32 or 64 bit OS
> > > > +    ; Install in default location
> > > > +    InetBgDL::Get "${BaseURLStubPing}v1/$ExitCode/$FirefoxLaunch/" \
> > > 
> > > As mentioned I'll ignore this for now.
> > Not sure what you are referring to
> 
> I'm referring to the TODO comment and Comment 15.
Please don't ignore this call in the review. The actual url that will be used will likely change and that is all I was asking to be ignored.

Comment 23

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

Comment on attachment 674630 [details] [diff] [review]
patch rev1

Going to clean this up a bit

Comment 24

[Brian R. Bondy [:bbondy]]

> Also, brand wouldn't work here since it isn't part of the filesystem path for the profile. 

Ya sorry I wasn't thinking about that. Ignore that review comment.

Comment 25

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

Attachment: patch rev2

Comment 26

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

Attachment: patch rev3

Forgot to address one review comment

Comment 27

[Brian R. Bondy [:bbondy]]

Comment on attachment 674793 [details] [diff] [review]
patch rev3

Review of attachment 674793 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/installer/windows/nsis/stub.nsi
@@ +888,5 @@
>  Function leaveInstall
>  # Need a ping?
>  FunctionEnd
>  
> +Function GetSecondsToDownload

nit: Please add a comment above this function to explain that it is not idempotent. I.e. That it calculates the amount of time between now and $SecondsToDownload and stores the results back into $SecondsToDownload.  For that reason it should only be called once for the purpose of determining the number of elapsed seconds to download.

@@ +1015,5 @@
>        ${If} $0 == 0
>        ${OrIf} $1 == 0
>          ; Use a timer so the UI has a chance to update
>          ${NSD_CreateTimer} DisplayDownloadError ${InstallIntervalMS}
>          Return

I think there should be this call before the return:
StrCpy $DownloadedAmount $3

Comment 28

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

(In reply to Brian R. Bondy [:bbondy] from comment #27)
> Comment on attachment 674793 [details] [diff] [review]
> patch rev3
> 
> Review of attachment 674793 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/installer/windows/nsis/stub.nsi
> @@ +888,5 @@
> >  Function leaveInstall
> >  # Need a ping?
> >  FunctionEnd
> >  
> > +Function GetSecondsToDownload
> 
> nit: Please add a comment above this function to explain that it is not
> idempotent. I.e. That it calculates the amount of time between now and
> $SecondsToDownload and stores the results back into $SecondsToDownload.  For
> that reason it should only be called once for the purpose of determining the
> number of elapsed seconds to download.
Will do

> @@ +1015,5 @@
> >        ${If} $0 == 0
> >        ${OrIf} $1 == 0
> >          ; Use a timer so the UI has a chance to update
> >          ${NSD_CreateTimer} DisplayDownloadError ${InstallIntervalMS}
> >          Return
> 
> I think there should be this call before the return:
> StrCpy $DownloadedAmount $3
That is for the case where the download has already finished and is handled by
>   ${If} $IsDownloadFinished != "true"
>     ${If} $2 == 0
>       ${NSD_KillTimer} OnDownload
>       StrCpy $IsDownloadFinished "true"
>       ; The first step of the install progress bar is determined by the
>       ; InstallProgressFirstStep define and provides the user with immediate
>       ; feedback.
>       StrCpy $InstallCounterStep ${InstallProgressFirstStep}
>+      Call GetSecondsToDownload
>+      StrCpy $DownloadedAmount $DownloadSize

Comment 29

[Brian R. Bondy [:bbondy]]

Agreed, thanks.

Comment 30

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

The URL as it stands right now will be

http://downloads-stats.mozilla.org/stub/v1/ExitCode/FirefoxLaunch/SecondsToDownload/DownloadedAmount/ExistingProfile/ExistingInstall/Locale/Channel

Comment 31

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

Some notes:

v1 - so we can easily deploy a v2

ExitCode can currently be one of the following:
ERR_SUCCESS 0
ERR_CANCEL_DOWNLOAD 10
ERR_INVALID_HANDLE 11
ERR_CERT_UNTRUSTED 12
ERR_CERT_ATTRIBUTES 13
ERR_CERT_UNTRUSTED_AND_ATTRIBUTES 14
ERR_CHECK_INSTALL_TIMEOUT 15
ERR_UNKNOWN 99

SecondsToDownload is self-explanatory


FirefoxLaunch can currently be one of the following:
0 - not launched (e.g. download cancelled)
1 - Firefox already running
2 - Firefox launched

DownloadAmount in KiB

ExistingProfile - 1 if there were Firefox profiles on this system prior to installation, 0 otherwise

ExistingInstall - 1 if installing on top of an existing install, 0 otherwise

Locale - self-explanatory (e.g. en-US)

Channel - self-explanatory (e.g. nightly, aurora, beta, or release. There is also unofficial but we shouldn't be seeing that often if at all)

Comment 32

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

Attachment: patch rev4 - updated to comments

Comment 33

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

Attachment: patch rev5 - changed url order

Updated URL
http://downloads-stats.mozilla.org/stub/v1/Channel/Locale/ExitCode/FirefoxLaunch/SecondsToDownload/DownloadedAmount/ExistingProfile/ExistingInstall/

Comment 34

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

Attachment: patch rev6 - update the url

Jake asked to use download-stats.mozilla.org instead of downloads-stats.mozilla.org

Comment 35

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

Attachment: Privacy notification screenshot

We might have to add a privacy notification for the ping and since time is very short I am putting together a patch to do so.

Comment 36

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

Attachment: add prompt patch rev1 (Not going to be used, see Bug 805575 Comment 27)

Brian, there are privacy notification concerns regarding this ping so I added a prompt and removed a couple of the data points to simplify the notification since I am going on vacation and this needs to be done asap. I am working with UX on the final strings so don't worry about the text. I plan on only having this turned on for the funnelcake test that is coming up and we can decide if we want it on all of the time after the privacy requirements are finalized.

Comment 37

[Brian R. Bondy [:bbondy]]

Comment on attachment 675022 [details] [diff] [review]
add prompt patch rev1 (Not going to be used, see Bug 805575 Comment 27)

Review of attachment 675022 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: browser/installer/windows/nsis/stub.nsi
@@ +125,5 @@
> +Var SecondsToDownload
> +Var DownloadedAmount
> +Var FirefoxLaunch
> +
> +!define PING_MSG "Help Mozilla by sending a summary of how this install went.$\n$\nHere's the info that we're gathering:$\nWhether the install was successful, and Firefox started afterwards.$\nHow long it took to download and the size of the download."

I would make changes here but overlooking as per the request.

Comment 38

[Brian R. Bondy [:bbondy]]

I wonder if it would be acceptable to put a checkbox defaulted to on in preferences for reporting anonymous telemetry data on the install process.

Comment 39

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

(In reply to Brian R. Bondy [:bbondy] from comment #38)
> I wonder if it would be acceptable to put a checkbox defaulted to on in
> preferences for reporting anonymous telemetry data on the install process.
Chicken Egg...

If the user hasn't installed yet how could the stub know?

Comment 40

[Brian R. Bondy [:bbondy]]

I'm not sure I understand what you mean, if a checkbox was acceptable, then after the install process, if the checkbox is on, then we'd send the ping without prompting the user.

Comment 41

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

(In reply to Brian R. Bondy [:bbondy] from comment #40)
> I'm not sure I understand what you mean, if a checkbox was acceptable, then
> after the install process, if the checkbox is on, then we'd send the ping
> without prompting the user.
So, what you're suggesting is we'd store the data we want from the ping and have the app pick it up after the install and report it. It could do but I am not interested in adding that complexity for this at this time.

Comment 42

[Brian R. Bondy [:bbondy]]

No I was just suggesting that we could add a checkbox into options, then after the install process is done, send the ping from inside the stub if the checkbox value was not turned off.

Comment 43

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

You are referring to the stub's options? If so, that page isn't displayed for the normal install path.

Comment 44

[Brian R. Bondy [:bbondy]]

Yes that's what I'm referring to, and yup I know it is not.  That's why I prefixed my original comment with: I wonder if it would be acceptable :D

Comment 45

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

(In reply to Brian R. Bondy [:bbondy] from comment #44)
> Yes that's what I'm referring to, and yup I know it is not.  That's why I
> prefixed my original comment with: I wonder if it would be acceptable :D
Sorry, got confused with the telemetry reference. Anyways, no. :D

Comment 46

[Brian R. Bondy [:bbondy]]

And if it wasn't privacy wise, then we could default it to off and just simply get less data reports, but still get some.

Comment 47

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

(In reply to Brian R. Bondy [:bbondy] from comment #46)
> And if it wasn't privacy wise, then we could default it to off and just
> simply get less data reports, but still get some.
Already understood.

Let's quit trying to find someway to get this in for the current study since there is no decent way at present. There is other work to be done and this can happen after the privacy requirements have been finalized.

Comment 48

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

Filed bug 805575 for getting the UX around the privacy notification

Comment 49

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Checkbox for send ping

Implemented checkbox for sending the ping. It is checked by default.

Another note, if you go to the options, uncheck the checkbox and then go back and then install, it'll still send the ping. That's because all checkboxes (not just this one) get reset when you go back. 

For the motivation of this task, please see Bug 805575 Comment 21 - Bug 805575 Comment 28

Comment 50

[Jim Mathies [:jimm]]

Comment on attachment 677016 [details] [diff] [review]
Patch v1 - Checkbox for send ping

Review of attachment 677016 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/installer/windows/nsis/stub.nsi
@@ +657,5 @@
>      ; plugin isn't included in the stub installer to lessen its size.
>      ClearErrors
>      ReadRegStr $0 HKLM "SYSTEM\CurrentControlSet\services\MozillaMaintenance" "ImagePath"
>      ${If} ${Errors}
> +      ${NSD_CreateCheckbox} ${OPTIONS_ITEM_EDGE_DU} 184u ${OPTIONS_ITEM_WIDTH_DU} \

Curious, what are these weird constants here? (swapping 175u with 184u?)

Comment 51

[Brian R. Bondy [:bbondy]]

It's the alignment with the other controls. So OPTIONS_ITEM_EDGE_DU puts it aligned with the "Destination Folder" text.  175 moves that checkbox down a bit.   See the epic ascii art in Bug 805575 Comment 28.

Comment 52

[Brian R. Bondy [:bbondy]]

If you notice discussion in bug 805575 about the text, just ignore that, I'll update this patch with the correct text once it's final.

Comment 53

[Brian R. Bondy [:bbondy]]

Three questions for *Johnath (well maybe not for you but you can probably press for the answers from others):
1) Is this the final text you want for the installer in the Options page? Tell Mozilla how this install goes
2) Is this the final URL to submit the pings? http://download-stats.mozilla.org/stub/v1/
3) Do you want this to land now on mozilla-central?

Comment 54

[Johnathan Nightingale [:johnath]]

(In reply to Brian R. Bondy [:bbondy] from comment #53)
> Three questions for *Johnath (well maybe not for you but you can probably
> press for the answers from others):
> 1) Is this the final text you want for the installer in the Options page?
> Tell Mozilla how this install goes

No. I think the text has evolved since you asked this question. In https://bugzilla.mozilla.org/show_bug.cgi?id=805575#c39 I think we've settled on new text.

> 2) Is this the final URL to submit the pings?
> http://download-stats.mozilla.org/stub/v1/

That accords with my read of bug 804233. jakem, do you agree?

> 3) Do you want this to land now on mozilla-central?

jakem, gilbert, anurag - are we ready to start seeing ping-enabled stubs on nightly?

Comment 55

[Jake Maul [:jakem]]

(In reply to Johnathan Nightingale [:johnath] from comment #54)
> > 2) Is this the final URL to submit the pings?
> > http://download-stats.mozilla.org/stub/v1/
> 
> That accords with my read of bug 804233. jakem, do you agree?

Sort of. Basically, this service does not care at all what path or query string you use. All that matters is http://download-stats.mozilla.org/. The rest is up to you... it will return a "200 OK" for everything. The hit will be logged, and metrics can process the log data as usual.

> > 3) Do you want this to land now on mozilla-central?
> 
> jakem, gilbert, anurag - are we ready to start seeing ping-enabled stubs on
> nightly?

IT/WebOps is ready for this.

Comment 56

[Brian R. Bondy [:bbondy]]

> IT/WebOps is ready for this.

Do we need signoff from anyone else for this to land?

Comment 57

[Brian R. Bondy [:bbondy]]

If Comment 54 had AND-semantics instead of OR-semantics, then we need feedback from gilbert and anurag for it to land.

Comment 58

[Johnathan Nightingale [:johnath]]

Comment 54 has AND semantics - I've sent Gilbert and Anurag an email reminder.

Metrics:

a) does this URL work? http://download-stats.mozilla.org/stub/v1/
b) are you ready for pings to start firing on nightly?

Comment 59

[Brian R. Bondy [:bbondy]]

We're good to go on a)

Comment 60

[Alex Keybl [:akeybl]]

(In reply to Brian R. Bondy [:bbondy] from comment #59)
> We're good to go on a)

When we're good to go on (b) as well, a=akeybl for landing on mozilla-aurora once landed on m-c.

Comment 61

[Anurag Phadke[:[e-mail]]]

(In reply to Johnathan Nightingale [:johnath] from comment #58)
a) is all IT/jakem and b) is all :bbondy + :rstrong. 

metrics is all ready.

Comment 62

[Johnathan Nightingale [:johnath]]

Brian - you're all clear to land, as I read it.

Comment 63

[Brian R. Bondy [:bbondy]]

m-c:
http://hg.mozilla.org/mozilla-central/rev/5a37fa4fe004
http://hg.mozilla.org/mozilla-central/rev/de2e5f230811

aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/5e042a560ba2
https://hg.mozilla.org/releases/mozilla-aurora/rev/aab747163346

Comment 64

[Brian R. Bondy [:bbondy]]

I'm not sure if we need to notify anyone here, but usually we don't land past mozilla-central when there are strings that need to be localized.  Something to look out for if we start offering localized aurora builds.

Comment 65

[Jason Smith [:jsmith]]

Brian - Is it worth flagging late-l10n on this bug then?

Comment 66

[Brian R. Bondy [:bbondy]]

Not sure, currently we're only doing the stub in en-us on Aurora.

Comment 67

[Axel Hecht [:Pike]]

This broke string freeze on aurora.

Comment 68

[Brian R. Bondy [:bbondy]]

I was asked by drivers to land on Aurora even know there is an English string. I think because the stub is English only.  Does this cause a problem?

Comment 69

[Brian R. Bondy [:bbondy]]

Maybe a good thing to do would be to move the string for Aurora only into the nsi and outside of the nsisstrings.properties file so the localization scripts don't see the string?

Comment 70

[Axel Hecht [:Pike]]

I think those are questions for akeybl, as they impact when we can start offering localized stubs.

Comment 71

[Brian R. Bondy [:bbondy]]

OK so options are:
1) Backout from Aurora
2) Move the string outside of the nsistrings.properties, I *think* the localization scripts wouldn't notice the string in that case.
3) Leave it as is
4-N) Something I didn't think of

Comment 72

[Jason Smith [:jsmith]]

(In reply to Brian R. Bondy [:bbondy] from comment #71)
> OK so options are:
> 1) Backout from Aurora
> 2) Move the string outside of the nsistrings.properties, I *think* the
> localization scripts wouldn't notice the string in that case.
> 3) Leave it as is
> 4-N) Something I didn't think of

Here's followup questions that might answer which option to go with above:

1. Are we currently planning to have localized stubs in Firefox 18?
2. What's the risk of breaking string freeze in this case?
3. Is it possible to move to Firefox 19 for localized stubs and ride the trains instead?
4. Are there any other options to consider?

Alex - Can you help address these questions?

Comment 73

[Alex Keybl [:akeybl]]

(In reply to Brian R. Bondy [:bbondy] from comment #71)
> OK so options are:
> 1) Backout from Aurora
> 2) Move the string outside of the nsistrings.properties, I *think* the
> localization scripts wouldn't notice the string in that case.
> 3) Leave it as is
> 4-N) Something I didn't think of

Let's leave this code in Aurora for now, move the string outside of nsistrings.properties, and only enable the ping functionality for the English installer to avoid breaking string freeze.

We'll continue to run ping experiments against the English stub installer for the time being.

Comment 74

[Alex Keybl [:akeybl]]

The above recommendation should only be for FF18 - leaving things as is for FF19 is fine.

Comment 75

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Aurora only for en-us only checkbox

[Approval Request Comment]
Bug caused by (feature/regressing bug #): this bug
User impact if declined: Whatever the effects of breaking localization is
Testing completed (on m-c, etc.): aurora only patch
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: Only includes the string for en-us builds.

Comment 76

[Jim Mathies [:jimm]]

Comment on attachment 677793 [details] [diff] [review]
Patch v1 - Aurora only for en-us only checkbox

Is the inline string here is approved by ux?

Comment 77

[Brian R. Bondy [:bbondy]]

(In reply to Jim Mathies [:jimm] from comment #76)
> Comment on attachment 677793 [details] [diff] [review]
> Patch v1 - Aurora only for en-us only checkbox
> 
> Is the inline string here is approved by ux?

I'm not sure about UX but we do have the appropriate approvals on the string. It is not introduced in this patch it is just being moved.

Comment 78

[Alex Keybl [:akeybl]]

Comment on attachment 677793 [details] [diff] [review]
Patch v1 - Aurora only for en-us only checkbox

[Triage Comment]
Approving to prevent breaking string freeze, but allowing us to run our ping experiments.

Comment 79

[Brian R. Bondy [:bbondy]]

Ping only sent in en-US builds: 
https://hg.mozilla.org/releases/mozilla-aurora/rev/59e0991e1174

Comment 80

[Jason Smith [:jsmith]]

Followup bug for this - bug 808110.

Comment 81

[Anurag Phadke[:[e-mail]]]

:rstrong - i grepped the logs on download-stats, the format mentioned in comment #30 and #31 doesn't match with actual data, some fields seem to be intermingled.

/stub/v1/ExitCode/FirefoxLaunch/SecondsToDownload/DownloadedAmount/ExistingProfile/ExistingInstall/Locale/Channel

/stub/v1/aurora/en-US/0/1/-361/20443/1/0/
/stub/v1/aurora/en-US/0/1/417/20443/1/1/
/stub/v1/nightly/en-US/0/2/7/19865/1/0/
/stub/v1/aurora/en-US/0/2/125/20443/1/0/

Comment 82

[Brian R. Bondy [:bbondy]]

Here's the corrected format:
http://downloads-stats.mozilla.org/stub/v1/Channel/Locale/ExitCode/FirefoxLaunch/SecondsToDownload/DownloadedAmount/ExistingProfile/ExistingInstall

Along with this information:

(In reply to Robert Strong [:rstrong] (vac 10/26-11/4) (do not email) from comment #31)
> Some notes:
> 
> v1 - so we can easily deploy a v2
> 
> ExitCode can currently be one of the following:
> ERR_SUCCESS 0
> ERR_CANCEL_DOWNLOAD 10
> ERR_INVALID_HANDLE 11
> ERR_CERT_UNTRUSTED 12
> ERR_CERT_ATTRIBUTES 13
> ERR_CERT_UNTRUSTED_AND_ATTRIBUTES 14
> ERR_CHECK_INSTALL_TIMEOUT 15
> ERR_UNKNOWN 99
> 
> SecondsToDownload is self-explanatory
> 
> 
> FirefoxLaunch can currently be one of the following:
> 0 - not launched (e.g. download cancelled)
> 1 - Firefox already running
> 2 - Firefox launched
> 
> DownloadAmount in KiB
> 
> ExistingProfile - 1 if there were Firefox profiles on this system prior to
> installation, 0 otherwise
> 
> ExistingInstall - 1 if installing on top of an existing install, 0 otherwise
> 
> Locale - self-explanatory (e.g. en-US)
> 
> Channel - self-explanatory (e.g. nightly, aurora, beta, or release. There is
> also unofficial but we shouldn't be seeing that often if at all)