Owner and definition for Signin.*Actions. Add one for Android.

Owner and definition for Signin.*Actions. Add one for Android.

BUG=357693
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277925

[Mike Lerman, 2014-06-04 20:07:06]

Hi Alexei,

Can you review this CL for me please?

Ruslan,

This CL will add the histogram that you can call for instrumenting the Android
sign-in flow. We can discuss offline how you'd want to call it.

[Alexei Svitkine (slow), 2014-06-05 17:23:16]

Is there a corresponding CL where this is implemented? I'd like to review the
two together.

[Mike Lerman, 2014-06-05 17:25:06]

On 2014/06/05 17:23:16, Alexei Svitkine wrote:
> Is there a corresponding CL where this is implemented? I'd like to review the
> two together.

I'm taking ownership of the Signin.*Action histograms which already exist
(references are in one_click_signin_helper.cc). I'm adding one new one also that
will be implemented for Clank, and that CL is being done by someone else. I'm
making sure the histogram's ready.

[Alexei Svitkine (slow), 2014-06-05 19:32:19]

Okay, let me know when the Clank CL is ready and I'll take a look then. (If you
want to land the ownership change earlier than that, then please split it out in
a separate CL - otherwise, just land them together when ready.)

[Mike Lerman, 2014-06-13 15:43:08]

On 2014/06/05 19:32:19, Alexei Svitkine wrote:
> Okay, let me know when the Clank CL is ready and I'll take a look then. (If
you
> want to land the ownership change earlier than that, then please split it out
in
> a separate CL - otherwise, just land them together when ready.)

Alexei, I think this is the related Clank CL.
https://chrome-internal-review.googlesource.com/#/c/165097/

[aruslan, 2014-06-13 21:44:45]

On 2014/06/13 15:43:08, Mike Lerman wrote:
> On 2014/06/05 19:32:19, Alexei Svitkine wrote:
> > Okay, let me know when the Clank CL is ready and I'll take a look then. (If
> you
> > want to land the ownership change earlier than that, then please split it
out
> in
> > a separate CL - otherwise, just land them together when ready.)
> 
> Alexei, I think this is the related Clank CL.
> https://chrome-internal-review.googlesource.com/#/c/165097/

Correction: Android-specific one is
https://chrome-internal-review.googlesource.com/166139

[Alexei Svitkine (slow), 2014-06-16 17:35:24]

https://codereview.chromium.org/319583002/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/319583002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:24243: +<histogram name="Signin"
enum="SigninHelperFlow">
I think this should be Signin.Actions.

https://codereview.chromium.org/319583002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:46974: +  <suffix
name="ExtensionInstallBubbleActions"/>
I'm not sure I understand these.

What does it mean for Signin.Actions.ExtensionInstallBubbleActions.Shown? What
infobar was shown to the user and how does that relate to the install bubble?

https://codereview.chromium.org/319583002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:46981: +  <affected-histogram
name="Signin"/>
Change this to Signin.Actions.

[Mike Lerman, 2014-06-16 17:41:59]

This is all existing and live C++ code, so I'm sadly limited in what scope of
changes I feel I can make without removing the history of existing data. Can I
rename the histograms and preserve the existing data?

https://codereview.chromium.org/319583002/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/319583002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:24243: +<histogram name="Signin"
enum="SigninHelperFlow">
On 2014/06/16 17:35:24, Alexei Svitkine wrote:
> I think this should be Signin.Actions.

I would think so too, but there are existing over a dozen histograms that all
have names like Signin.SettingsActions or Signin.StartPageActions. I don't want
to lose the history we already have.

https://codereview.chromium.org/319583002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:46974: +  <suffix
name="ExtensionInstallBubbleActions"/>
On 2014/06/16 17:35:24, Alexei Svitkine wrote:
> I'm not sure I understand these.
> 
> What does it mean for Signin.Actions.ExtensionInstallBubbleActions.Shown? What
> infobar was shown to the user and how does that relate to the install bubble?

This would mean that the user was directed to the common Signin-and-Sync flow,
originating from the Extension Installer Bubble. Once in this flow, the user was
Shown the infobar (e.g. the bubble).

https://codereview.chromium.org/319583002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:46981: +  <affected-histogram
name="Signin"/>
On 2014/06/16 17:35:24, Alexei Svitkine wrote:
> Change this to Signin.Actions.

As above.

[Alexei Svitkine (slow), 2014-06-16 17:52:44]

On 2014/06/16 17:41:59, Mike Lerman wrote:
> This is all existing and live C++ code, so I'm sadly limited in what scope of
> changes I feel I can make without removing the history of existing data. Can I
> rename the histograms and preserve the existing data?

I see. I think there's a disconnect with the Android CL, though since it's using
Signin.Actions.AndroidAccountConsistencyFirstRunActions which doesn't match this
change.

https://codereview.chromium.org/319583002/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/319583002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:46974: +  <suffix
name="ExtensionInstallBubbleActions"/>
On 2014/06/16 17:41:59, Mike Lerman wrote:
> On 2014/06/16 17:35:24, Alexei Svitkine wrote:
> > I'm not sure I understand these.
> > 
> > What does it mean for Signin.Actions.ExtensionInstallBubbleActions.Shown?
What
> > infobar was shown to the user and how does that relate to the install
bubble?
> 
> This would mean that the user was directed to the common Signin-and-Sync flow,
> originating from the Extension Installer Bubble. Once in this flow, the user
was
> Shown the infobar (e.g. the bubble).

Wait. An infobar is not a bubble, so "Shown the infobar (e.g. the bubble)"
doesn't make sense to me. Perhaps the description for the "Shown" entry needs to
change?

Also, the above information (i.e. what this means) needs to be captured
somewhere - maybe in the description for the "Signin" top-level histogram -
perhaps by explaining an example like you did in this reply to me.

https://codereview.chromium.org/319583002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:46980: +  <suffix
name="WebstoreInstallActions"/>
How about UnknownActions?

[Mike Lerman, 2014-06-16 20:20:37]

Hi Ruslan,

Would you be able to change the name of the histogram to match the pattern we
have with the other Signin.* histograms? Specifically, remove the ".Actions"
from the middle of the histogram name.

[aruslan, 2014-06-16 20:27:49]

On 2014/06/16 20:20:37, Mike Lerman wrote:
> Hi Ruslan,
> 
> Would you be able to change the name of the histogram to match the pattern we
> have with the other Signin.* histograms? Specifically, remove the ".Actions"
> from the middle of the histogram name.

Do you mean specifically a Mirror-specific
Signin.Actions.AndroidAccountConsistencyFirstRunActions?
What should it say? Signin.AndroidAccountConsistencyFirstRunActions?

[Mike Lerman, 2014-06-16 20:29:00]

On 2014/06/16 20:27:49, aruslan wrote:
> On 2014/06/16 20:20:37, Mike Lerman wrote:
> > Hi Ruslan,
> > 
> > Would you be able to change the name of the histogram to match the pattern
we
> > have with the other Signin.* histograms? Specifically, remove the ".Actions"
> > from the middle of the histogram name.
> 
> Do you mean specifically a Mirror-specific
> Signin.Actions.AndroidAccountConsistencyFirstRunActions?
> What should it say? Signin.AndroidAccountConsistencyFirstRunActions?

"Signin.AndroidAccountConsistencyFirstRunActions", yes.

[aruslan, 2014-06-16 20:35:50]

On 2014/06/16 20:29:00, Mike Lerman wrote:
> On 2014/06/16 20:27:49, aruslan wrote:
> > On 2014/06/16 20:20:37, Mike Lerman wrote:
> > > Hi Ruslan,
> > > 
> > > Would you be able to change the name of the histogram to match the pattern
> we
> > > have with the other Signin.* histograms? Specifically, remove the
".Actions"
> > > from the middle of the histogram name.
> > 
> > Do you mean specifically a Mirror-specific
> > Signin.Actions.AndroidAccountConsistencyFirstRunActions?
> > What should it say? Signin.AndroidAccountConsistencyFirstRunActions?
> 
> "Signin.AndroidAccountConsistencyFirstRunActions", yes.

Landed to the internal trunk,
https://chrome-internal-review.googlesource.com/166256

[Mike Lerman, 2014-06-17 14:00:29]

Ruslan changed the name of the Android CL to match the pattern provided.

https://codereview.chromium.org/319583002/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/319583002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:46974: +  <suffix
name="ExtensionInstallBubbleActions"/>
On 2014/06/16 17:52:44, Alexei Svitkine wrote:
> On 2014/06/16 17:41:59, Mike Lerman wrote:
> > On 2014/06/16 17:35:24, Alexei Svitkine wrote:
> > > I'm not sure I understand these.
> > > 
> > > What does it mean for Signin.Actions.ExtensionInstallBubbleActions.Shown?
> What
> > > infobar was shown to the user and how does that relate to the install
> bubble?
> > 
> > This would mean that the user was directed to the common Signin-and-Sync
flow,
> > originating from the Extension Installer Bubble. Once in this flow, the user
> was
> > Shown the infobar (e.g. the bubble).
> 
> Wait. An infobar is not a bubble, so "Shown the infobar (e.g. the bubble)"
> doesn't make sense to me. Perhaps the description for the "Shown" entry needs
to
> change?
> 
> Also, the above information (i.e. what this means) needs to be captured
> somewhere - maybe in the description for the "Signin" top-level histogram -
> perhaps by explaining an example like you did in this reply to me.
> 

I've modified the comments here to make them make more sense, and described in
the histogram summary what the suffixes do.

https://codereview.chromium.org/319583002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:46980: +  <suffix
name="WebstoreInstallActions"/>
On 2014/06/16 17:52:44, Alexei Svitkine wrote:
> How about UnknownActions?

Added.

[Alexei Svitkine (slow), 2014-06-17 15:02:58]

lgtm

https://codereview.chromium.org/319583002/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/319583002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:24246: +    Tracks user interactions as
they sign in through a flow. The suffix     of
Nit: Extra whitespace before "of"

[Mike Lerman, 2014-06-17 15:04:36]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-06-17 15:05:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlerman@chromium.org/319583002/40001

[commit-bot: I haz the power, 2014-06-17 19:29:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-17 19:29:42]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[Mike Lerman, 2014-06-17 19:30:07]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-06-17 19:32:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlerman@chromium.org/319583002/40001

[commit-bot: I haz the power, 2014-06-17 19:45:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-17 19:45:11]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[Mike Lerman, 2014-06-17 20:12:37]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-06-17 20:14:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlerman@chromium.org/319583002/60001

[commit-bot: I haz the power, 2014-06-18 00:37:07]

Change committed as 277925