1468843 - Only record docgroup count telemetry for http(s)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Andreas Farre [:farre]]

Attachment: 0001-Bug-1468843-Skip-collecting-docgroup-count-for-about.patch

Comment 2

[Andreas Farre [:farre]]

And really, I wonder if we should turn this around and say that we should only count http/https schemed tabs? Now we'd be counting not only about, but also file, moz-extension, etc.

Comment 3

[Andreas Farre [:farre]]

Attachment: 0001-Bug-1468843-Only-record-docgroup-count-for-http-s-.-.patch

Changed to only count http(s)

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8985659 [details] [diff] [review]
0001-Bug-1468843-Only-record-docgroup-count-for-http-s-.-.patch

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

::: dom/base/nsDocument.cpp
@@ +8485,5 @@
> +static bool
> +HasHttpScheme(nsIURI* aURI)
> +{
> +  bool isHttpish = false;
> +  return (!NS_FAILED(aURI->SchemeIs("http", &isHttpish)) && isHttpish) ||

nit: NS_SUCCEEDED

@@ +8493,5 @@
>  void
>  nsIDocument::OnPageHide(bool aPersisted, EventTarget* aDispatchStartTarget)
>  {
> +  if (GetDocGroup() && Telemetry::CanRecordExtended() &&
> +      IsTopLevelContentDocument() && GetDocumentURI() &&

Can we put a nsCOMPtr<nsIURI> docUri = GetDocumentURI() on the previous line to make it more clear that we're not doing extra work here?

Comment 5

[Andreas Farre [:farre]]

(In reply to :Nika Layzell from comment #4)
> Comment on attachment 8985659 [details] [diff] [review]
> 0001-Bug-1468843-Only-record-docgroup-count-for-http-s-.-.patch
> 
> Review of attachment 8985659 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsDocument.cpp
> @@ +8485,5 @@
> > +static bool
> > +HasHttpScheme(nsIURI* aURI)
> > +{
> > +  bool isHttpish = false;
> > +  return (!NS_FAILED(aURI->SchemeIs("http", &isHttpish)) && isHttpish) ||
> 
> nit: NS_SUCCEEDED

Sure thing.

> @@ +8493,5 @@
> >  void
> >  nsIDocument::OnPageHide(bool aPersisted, EventTarget* aDispatchStartTarget)
> >  {
> > +  if (GetDocGroup() && Telemetry::CanRecordExtended() &&
> > +      IsTopLevelContentDocument() && GetDocumentURI() &&
> 
> Can we put a nsCOMPtr<nsIURI> docUri = GetDocumentURI() on the previous line
> to make it more clear that we're not doing extra work here?

Is it the overhead of the extra call to GetDocumentURI we're worried about here? Looking at the disassembly the compiler seems to be smart enough to optimize away the extra call. Otoh, introducing the construction of an nsCOMPtr will require more work. How about, if I move the nullcheck to HasHttpScheme? That yields exactly the same result with what we have now, but without needing to construct the COM pointer.

Comment 6

[Andreas Farre [:farre]]

Attachment: 0001-Bug-1468843-Only-record-docgroup-count-for-http-s-.-.patch

I.e. this way.

Comment 7

[Andreas Farre [:farre]]

Attachment: 0001-Bug-1468843-Only-record-docgroup-count-for-http-s-.-.patch

Comment 8

[Nika Layzell [:nika] (ni? for response)]

We might actually want to use different names for these new values so we don't mix the data together.

Comment 9

[Andreas Farre [:farre]]

Attachment: 0001-Bug-1468843-Add-telemetry-that-only-record-docgroup-.patch

(In reply to :Nika Layzell from comment #8)
> We might actually want to use different names for these new values so we
> don't mix the data together.

Done, I'll do the data-review as soon as you've had a chance to look at it. 

In the patch change things around a bit and add the collection of ACTIVE_HTTP_DOCGROUPS_PER_TABGROUP and TOTAL_HTTP_DOCGROUPS_PER_TABGROUP, and keep ACTIVE_DOCGROUPS_PER_TABGROUP and TOTAL_DOCGROUPS_PER_TABGROUP, but it is pretty much how one would expect it.

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8986695 [details] [diff] [review]
0001-Bug-1468843-Add-telemetry-that-only-record-docgroup-.patch

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

::: dom/base/nsDocument.cpp
@@ +8515,1 @@
>        Telemetry::Accumulate(Telemetry::ACTIVE_DOCGROUPS_PER_TABGROUP,

Let's just kill ACTIVE and TOTAL without HTTP. I dopn't think they're very useful so I see no reason to keep them around.

Comment 11

[Andreas Farre [:farre]]

Attachment: 0001-Bug-1468843-Change-telemetry-to-only-record-docgroup.patch

Carrying over Nika's r+. The latest change is only the removal of {ACTIVE, TOTAL}_DOCGROUPS_PER_TABGROUP.

:chutten, what's the processes for changing a probe? The change is only a guard that prohibits non-http-ish from being accumulated, and the new probe names is only to keep new data from mixing with old data. So in all sense and purposes a small bug fix.

Comment 12

[Chris H-C :chutten]

Comment on attachment 8987592 [details] [diff] [review]
0001-Bug-1468843-Change-telemetry-to-only-record-docgroup.patch

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

As soon as you change the name, it's as though it's a completely new probe. You can change anything you'd like about it right now.

So it might be a good idea to ensure that the current bucket distribution is giving you the information you need (https://mzl.la/2KpTWjs), because this would be an excellent time to change those things... though it seems as though 20 buckets with a high of 50 seems to be filled pretty nicely (to me), so there's no pressing need. (Not like, for example, TELEMETRY_SEND_SUCCESS which could do with a "low" of at least 10: https://mzl.la/2KclfPi)

::: toolkit/components/telemetry/Histograms.json
@@ +13813,3 @@
>      "record_in_processes": ["content"],
>      "alert_emails": ["[e-mail]"],
>      "bug_numbers": [1441972],

Should update the bug_numbers of this probe, too.

Comment 13

[Chris H-C :chutten]

As for changing what it is collecting and how that interacts with Data Collection Review...

...in the general case, changing a probe means it's a new Data Collection and so it is subject to a new review. In this case, I've reviewed the review we underwent on bug 1441972 and it still applies to the changed collection, so you are good to proceed. You did exactly the right thing in asking :)

Comment 14

[Andreas Farre [:farre]]

Attachment: 0001-Bug-1468843-Change-telemetry-to-only-record-docgroup.patch

Added this #bug to both of the probes, thanks for catching that, and thanks for the help!

Just a final question then, do you want me to keep the "data-review=chutten" in the patch commit message as well?

Comment 15

[Chris H-C :chutten]

Sure, I have no problem with that.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/91dce758d71e
Change telemetry to only record docgroup count for http(s). r=nika,data-review=chutten

Comment 17

[Andreas Farre [:farre]]

Comment on attachment 8987745 [details] [diff] [review]
0001-Bug-1468843-Change-telemetry-to-only-record-docgroup.patch

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1441972 is where we initially added a probe similar to this, that also was uplifted to the beta at that time.
[User impact if declined]:
We get telemetry later.
[Is this code covered by automated tests?]:
No.
[Has the fix been verified in Nightly?]:
11 hours and counting.
[Needs manual test from QE? If yes, steps to reproduce]: 
No
[List of other uplifts needed for the feature/fix]:
None
[Is the change risky?]:
No.
[Why is the change risky/not risky?]:
This patch renames two probes and adds a guard to limit the amount of collected telemetry, and doesn't introduce any actual new behaviour.
[String changes made/needed]:
No

Comment 18

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/91dce758d71e

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8987745 [details] [diff] [review]
0001-Bug-1468843-Change-telemetry-to-only-record-docgroup.patch

Seems low-risk, and verified to be working in Nightly. Has data review. Let's try it in beta 5.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/69bd2a456d6b