Closed Bug 802734 Opened 7 years ago Closed 7 years ago

Add pings for metrics to the stub installer

Categories

(Firefox :: Installer, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 19
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: robert.strong.bugs, Assigned: robert.strong.bugs)

References

Details

(Keywords: late-l10n, Whiteboard: [stub+])

Attachments

(5 files, 5 obsolete files)

16.44 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
32.28 KB, image/png
Details
12.23 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
5.70 KB, patch
jimm
: review+
Details | Diff | Splinter Review
5.26 KB, patch
jimm
: review+
Details | Diff | Splinter Review
I'll add more details later
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.
(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.
(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.
Flags: needinfo?(chrismore.bugzilla)
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.
Flags: needinfo?(chrismore.bugzilla)
(adding daniel and xavier to shed light on collecting data on metrics servers)
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?
Whiteboard: [stub+]
It is for the expedited rev2 to evaluate the stub that we discussed on Tuesday.
:rstrong - should we have a 30-meeting with the folks on this bug and Stacy to discuss the pings, collection server and privacy?
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!
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.
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.
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!
Oh hey, I think I should take privacy point on this rather than Stacy. Thanks!
No longer depends on: 804231
Attached patch patch rev1 (obsolete) — Splinter Review
This doesn't get us the world but this does get us what we need for the time being.
Attachment #674630 - Flags: review?(netzen)
Brian, please ignore the url used. I'll get the final url that will be used later.
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.
No major concerns, mostly just questions and "should we do X to get data of type Y?" type questions.
(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
> > @@ +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.
(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.
isn't required to match brand that is
(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 on attachment 674630 [details] [diff] [review]
patch rev1

Going to clean this up a bit
Attachment #674630 - Flags: review?(netzen)
> 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.
Attached patch patch rev2 (obsolete) — Splinter Review
Attachment #674630 - Attachment is obsolete: true
Attachment #674792 - Flags: review?(netzen)
Attached patch patch rev3 (obsolete) — Splinter Review
Forgot to address one review comment
Attachment #674792 - Attachment is obsolete: true
Attachment #674792 - Flags: review?(netzen)
Attachment #674793 - Flags: review?(netzen)
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
Attachment #674793 - Flags: review?(netzen) → review+
(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
Agreed, thanks.
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)
Attached patch patch rev4 - updated to comments (obsolete) — Splinter Review
Attachment #674793 - Attachment is obsolete: true
Attachment #674819 - Flags: review+
Status: NEW → ASSIGNED
Jake asked to use download-stats.mozilla.org instead of downloads-stats.mozilla.org
Attachment #674845 - Attachment is obsolete: true
Attachment #674856 - Flags: review+
Attachment #674856 - Attachment description: patch rev5 - update the url → patch rev6 - update the url
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.
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.
Attachment #675022 - Flags: review?(netzen)
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.
Attachment #675022 - Flags: review?(netzen) → review+
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.
(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?
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.
(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.
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.
You are referring to the stub's options? If so, that page isn't displayed for the normal install path.
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
(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
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.
(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.
Depends on: 805575
Filed bug 805575 for getting the UX around the privacy notification
Attachment #675022 - Attachment description: add prompt patch rev1 → add prompt patch rev1 (Not going to be used, see Bug 805575 Comment 27)
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
Attachment #677016 - Flags: review?(jmathies)
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?)
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.
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.
Attachment #677016 - Flags: review?(jmathies) → review+
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?
(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?
(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.
> IT/WebOps is ready for this.

Do we need signoff from anyone else for this to land?
If Comment 54 had AND-semantics instead of OR-semantics, then we need feedback from gilbert and anurag for it to land.
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?
We're good to go on a)
(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.
Attachment #674856 - Flags: approval-mozilla-aurora+
Attachment #677016 - Flags: approval-mozilla-aurora+
(In reply to Johnathan Nightingale [:johnath] from comment #58)
a) is all IT/jakem and b) is all :bbondy + :rstrong. 

metrics is all ready.
Brian - you're all clear to land, as I read it.
Target Milestone: --- → Firefox 19
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Keywords: verifyme
QA Contact: jsmith
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.
Brian - Is it worth flagging late-l10n on this bug then?
Not sure, currently we're only doing the stub in en-us on Aurora.
This broke string freeze on aurora.
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?
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?
I think those are questions for akeybl, as they impact when we can start offering localized stubs.
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
Keywords: late-l10n
(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?
Flags: needinfo?(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.
Flags: needinfo?(akeybl) needinfo?(akeybl) → needinfo+
The above recommendation should only be for FF18 - leaving things as is for FF19 is fine.
[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.
Attachment #677793 - Flags: review?(jmathies)
Attachment #677793 - Flags: approval-mozilla-aurora?
Attachment #677016 - Attachment description: Patch v1. → Patch v1 - Aurora only for en-us only checkbox
Attachment #677016 - Attachment description: Patch v1 - Aurora only for en-us only checkbox → Patch v1 - Checkbox for send ping
Attachment #677793 - Attachment description: Patch v1 → Patch v1 - Aurora only for en-us only checkbox
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?
Attachment #677793 - Flags: review?(jmathies) → review+
(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 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.
Attachment #677793 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 808110
Followup bug for this - bug 808110.
Keywords: verifyme
Whiteboard: [stub+] → [stub+] [qa verification blocked]
Keywords: verifyme
Whiteboard: [stub+] [qa verification blocked] → [stub+]
: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/
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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)
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Keywords: verifyme
QA Contact: jsmith
Depends on: 810509
Status: RESOLVED → VERIFIED
Depends on: 811120
Depends on: 811646
Depends on: 812223
You need to log in before you can comment on or make changes to this bug.