Closed Bug 1468843 Opened 3 years ago Closed 3 years ago

Only record docgroup count telemetry for http(s)

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox62 --- fixed
firefox63 --- fixed

People

(Reporter: farre, Assigned: farre)

Details

Attachments

(1 file, 6 obsolete files)

No description provided.
Assignee: nobody → afarre
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.
Changed to only count http(s)
Attachment #8985505 - Attachment is obsolete: true
Attachment #8985505 - Flags: review?(nika)
Attachment #8985659 - Flags: review?(nika)
Summary: Don't collect docgroup count telemetry for about pages → Only record docgroup count telemetry for http(s)
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?
Attachment #8985659 - Flags: review?(nika) → review+
(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.
Flags: needinfo?(nika)
I.e. this way.
Attachment #8985659 - Attachment is obsolete: true
Flags: needinfo?(nika)
Attachment #8985932 - Flags: review?(nika)
Attachment #8985932 - Attachment is obsolete: true
Attachment #8985932 - Flags: review?(nika)
Attachment #8985934 - Flags: review?(nika)
Attachment #8985934 - Flags: review?(nika) → review+
We might actually want to use different names for these new values so we don't mix the data together.
(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.
Attachment #8985934 - Attachment is obsolete: true
Attachment #8986695 - Flags: review?(nika)
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.
Attachment #8986695 - Flags: review?(nika) → review+
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.
Attachment #8986695 - Attachment is obsolete: true
Flags: needinfo?(chutten)
Attachment #8987592 - Flags: review+
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": ["farre@mozilla.com"],
>      "bug_numbers": [1441972],

Should update the bug_numbers of this probe, too.
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 :)
Flags: needinfo?(chutten)
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?
Attachment #8987592 - Attachment is obsolete: true
Flags: needinfo?(chutten)
Attachment #8987745 - Flags: review+
Sure, I have no problem with that.
Flags: needinfo?(chutten)
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/91dce758d71e
Change telemetry to only record docgroup count for http(s). r=nika,data-review=chutten
Keywords: checkin-needed
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
Attachment #8987745 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/91dce758d71e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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.
Attachment #8987745 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.