1133521 - Enable BHR on Beta

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Roberto Agostino Vitillo (:rvitillo)]

We would like to enable BHR on Beta for about 1% of the population, that should give us enough data without overloading our backend infrastructure.

Comment 1

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: 1133521 - Enable BHR on Beta

Comment 2

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: 1133521

Comment 3

[Vladan Djeric (:vladan)]

Comment on attachment 8565364 [details] [diff] [review]
1133521

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

::: xpcom/threads/BackgroundHangMonitor.cpp
@@ +453,5 @@
>    return nullptr;
>  }
>  
> +bool
> +BackgroundHangMonitor::ShouldDisableOnBeta(nsAdoptingCString &clientID) {

why nsAdoptingCString& ? why not const nsCString& ?

@@ +456,5 @@
> +bool
> +BackgroundHangMonitor::ShouldDisableOnBeta(nsAdoptingCString &clientID) {
> +  MOZ_ASSERT(clientID, "clientID is null");
> +  const char *suffix = clientID.get() + clientID.Length() - 4;
> +  return strtol(suffix, NULL, 16) % 100; // Activate BHR only for ~ 1% of the Beta population

make the 1% a define at the top of the file

@@ +464,5 @@
> +BackgroundHangMonitor::IsDisabled() {
> +#ifdef MOZ_ENABLE_BACKGROUND_HANG_MONITOR
> +  return BackgroundHangManager::sDisabled;
> +#else
> +  return true

missing semi-colon

@@ +487,5 @@
>    MOZ_ASSERT(!BackgroundHangManager::sInstance, "Already initialized");
> +
> +  if (NS_STRINGIFY(MOZ_UPDATE_CHANNEL) == "beta") {
> +    auto clientID = Preferences::GetCString("toolkit.telemetry.cachedClientID");
> +    if (clientID && BackgroundHangMonitor::ShouldDisableOnBeta(clientID)) {

What happens when the user opts-out of FHR collection during the first session or resets their profile? I think clientID would never get set, thus enabling BHR during all future sessions?

::: xpcom/threads/BackgroundHangMonitor.h
@@ +110,5 @@
>  private:
>    RefPtr<BackgroundHangThread> mThread;
>  
> +  static void PrefCallback(const char *, void *);
> +  static bool ShouldDisableOnBeta(nsAdoptingCString &);

nit: ShouldEnableOnBeta? see next comment

Comment 4

[Roberto Agostino Vitillo (:rvitillo)]

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #3)
> Comment on attachment 8565364 [details] [diff] [review]
> 1133521
> 
> Review of attachment 8565364 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/threads/BackgroundHangMonitor.cpp
> @@ +453,5 @@
> >    return nullptr;
> >  }
> >  
> > +bool
> > +BackgroundHangMonitor::ShouldDisableOnBeta(nsAdoptingCString &clientID) {
> 
> why nsAdoptingCString& ? why not const nsCString& ?

Preferences::GetCString returns a nsAdoptingCString.

> @@ +487,5 @@
> >    MOZ_ASSERT(!BackgroundHangManager::sInstance, "Already initialized");
> > +
> > +  if (NS_STRINGIFY(MOZ_UPDATE_CHANNEL) == "beta") {
> > +    auto clientID = Preferences::GetCString("toolkit.telemetry.cachedClientID");
> > +    if (clientID && BackgroundHangMonitor::ShouldDisableOnBeta(clientID)) {
> 
> What happens when the user opts-out of FHR collection during the first
> session or resets their profile? I think clientID would never get set, thus
> enabling BHR during all future sessions?

Good point. I addressed this in the updated patch.

> ::: xpcom/threads/BackgroundHangMonitor.h
> @@ +110,5 @@
> >  private:
> >    RefPtr<BackgroundHangThread> mThread;
> >  
> > +  static void PrefCallback(const char *, void *);
> > +  static bool ShouldDisableOnBeta(nsAdoptingCString &);
> 
> nit: ShouldEnableOnBeta? see next comment

Which comment?

Comment 5

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: 1133521

Comment 6

[Vladan Djeric (:vladan)]

> > why nsAdoptingCString& ? why not const nsCString& ?
> 
> Preferences::GetCString returns a nsAdoptingCString.

I think Adopting[C]Strings are internal types used only for return types, so they're not used in function parameters. This method will only be called with the return values from Preferences::GetString() but it should still accept any CString.

> > nit: ShouldEnableOnBeta? see next comment
> 
> Which comment?

I had written a commment (and then deleted it) about having BHR default to disabled on Beta, and then only enabling it after we get the clientID and confirm the client is one of the 1%. But I changed my mind about that

Comment 7

[Vladan Djeric (:vladan)]

Comment on attachment 8566461 [details] [diff] [review]
1133521

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

::: xpcom/threads/BackgroundHangMonitor.cpp
@@ +34,3 @@
>  namespace mozilla {
>  
> +class Observer : public nsIObserver

Wouldn't it be simpler to have BackgroundHangManager inherit from nsIObserver instead?

Comment 8

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: 1133521

Comment 9

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: 1133521

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d0237f28533

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/2bac1ea5b5c6

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/integration/fx-team/rev/e38e66a9b55d

https://treeherder.mozilla.org/logviewer.html#?job_id=2120552&repo=fx-team

Comment 12

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: 1133521

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9abcd778004

Comment 13

[Carsten Book [:Tomcat]]

Hi, this failed to apply:

patching file toolkit/components/telemetry/Telemetry.cpp
Hunk #1 FAILED at 2898
1 out of 1 hunks FAILED -- saving rejects to file toolkit/components/telemetry/Telemetry.cpp.rej
patch failed, unable to continue (try -v)



could you take a look, thanks!

Comment 14

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: 1133521

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/34ca2a2e09ea

Comment 16

[Carsten Book [:Tomcat]]

sorry had to back this out for causing crashes like https://treeherder.mozilla.org/logviewer.html#?job_id=2176938&repo=fx-team on android

Comment 17

[Ryan VanderMeulen [:RyanVM]]

This caused widespread problems across multiple platforms. Please run a full Try push across all platforms before attempting to re-land.

Comment 18

[Roberto Agostino Vitillo (:rvitillo)]

Will do, sorry about that.

Comment 19

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: 1133521

Seems the issue was that BHR was getting enabled also on Debug builds.

Comment 20

[Roberto Agostino Vitillo (:rvitillo)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5dd61d7e7225

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/b4e7616d16b4

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b4e7616d16b4

Comment 23

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: 1133521_beta

Approval Request Comment
[Feature/regressing bug #]: 1133521
[User impact if declined]: None
[Describe test coverage new/current, TreeHerder]: landed on m-c on March-10 with tests
[Risks and why]: Browser might be slightly less power friendly for a small fraction of the population that is going to have BHR enabled.
[String/UUID change made/needed]: None

Comment 24

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: 1133521_aurora

Approval Request Comment
[Feature/regressing bug #]: 1133521
[User impact if declined]: None
[Describe test coverage new/current, TreeHerder]: landed on m-c on March-10 with tests
[Risks and why]: Low risk, this patch affects only Beta
[String/UUID change made/needed]: None

Comment 25

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8578683 [details] [diff] [review]
1133521_beta

We're at the end of the 37 Beta cycle with only one Beta build left. I think we should defer this work to 38 Beta. 38 Beta 1 is scheduled to release on Apr 2, 2015. Beta-

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e84f4f61eea1