1281793 - Remove some non-used telemetry IDs

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P4)

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch 1 - expired keys in nsBrowserApp

Comment 2

[Andrea Marchesini [:baku]]

Attachment: patch 2 - network/cache

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 3 - network/cache (2)

Comment 4

[Andrea Marchesini [:baku]]

Attachment: patch 3 - network/cache (2)

Comment 5

[Andrea Marchesini [:baku]]

Attachment: patch 4 - random things

Comment 6

[Andrea Marchesini [:baku]]

Attachment: patch 5 - random things (2)

Comment 7

[Andrea Marchesini [:baku]]

Attachment: patch 6 - Search and other stuff

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8764588 [details] [diff] [review]
patch 1 - expired keys in nsBrowserApp

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

::: toolkit/components/telemetry/Histograms.json
@@ -774,5 @@
>      "n_buckets": 21,
>      "description": "Number of low-commit-space events fired since last ping",
>      "cpp_guard": "XP_WIN"
>    },
> -  "EARLY_GLUESTARTUP_READ_OPS": {

Also remove them from histogram-whitelists.json.

Comment 9

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8764589 [details] [diff] [review]
patch 2 - network/cache

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

::: toolkit/components/telemetry/Histograms.json
@@ -1657,5 @@
>      "expires_in_version": "never",
>      "kind": "boolean",
>      "description": "Fraction of sockets that used a nsConnectionEntry with history - size 300."
>    },
> -  "DISK_CACHE_CORRUPT_DETAILS": {

Also remove them from histogram-whitelists.json.

Comment 10

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8764619 [details] [diff] [review]
patch 6 - Search and other stuff

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

::: toolkit/components/telemetry/Histograms.json
@@ -5093,5 @@
>      "kind": "enumerated",
>      "n_values": 10,
>      "description": "Track click count on about:newtab tiles per index (0-8). For non-default row or column configurations all clicks into the '9' bucket. *** No longer needed (bug 1156565). Delete histogram and accumulation code! ***"
>    },
> -  "BROWSERPROVIDER_XUL_IMPORT_TIME": {

Also remove them from histogram-whitelists.json.

@@ -6732,5 @@
> -    "kind": "linear",
> -    "low": 5,
> -    "high": 145,
> -    "n_buckets": 30,
> -    "description": "The number of distinct pairs (first-party site, third-party site attempting to set cookie) for which the third-party cookie has been accepted. Sites are considered identical if they have the same eTLD + 1. Measures are normalized per 24h. *** No longer needed (bug 1156565). Delete histogram and accumulation code! ***"

Lets make this bug block bug 1156565 then.

Comment 11

[Chris H-C :chutten]

Comment on attachment 8764591 [details] [diff] [review]
patch 3 - network/cache (2)

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

Remove the probes from histogram_whitelist.

Also, please seek an r? from a cache service dev, as I'm not sure I understand the syntax around those locks :S

Comment 12

[Chris H-C :chutten]

Comment on attachment 8764603 [details] [diff] [review]
patch 4 - random things

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

Remove probes from histograms_whitelist

Comment 13

[Chris H-C :chutten]

Comment on attachment 8764615 [details] [diff] [review]
patch 5 - random things (2)

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

And from the whitelists.

::: dom/base/nsGlobalWindow.cpp
@@ -12463,1 @@
>      gTimeoutsRecentlySet = 0;

gTimeoutsRecentlySet is never read except for telemetry accumulation. I guess it should be removed.

::: layout/base/nsPresShell.cpp
@@ +9792,5 @@
>    if (mDocument->GetRootElement()) {
>      TimeDuration elapsed = TimeStamp::Now() - timerStart;
>      int32_t intElapsed = int32_t(elapsed.ToMilliseconds());
>  
> +    if (mDocument->GetRootElement()->IsXULElement() && mIsActive) {

shouldn't this be if(!mDocument->GetRootElement()->IsXULElement() ?

Comment 14

[Andrea Marchesini [:baku]]

Attachment: patch 3 - network/cache (2)

Comment 15

[Andrea Marchesini [:baku]]

Attachment: patch 4 - random things

Comment 16

[Andrea Marchesini [:baku]]

Attachment: patch 5 - random things (2)

Comment 17

[Chris H-C :chutten]

Comment on attachment 8764849 [details] [diff] [review]
patch 4 - random things

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

LGTM

Comment 18

[Chris H-C :chutten]

Comment on attachment 8764855 [details] [diff] [review]
patch 5 - random things (2)

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

LGTM

Comment 19

[Jason Duell]

Comment on attachment 8764848 [details] [diff] [review]
patch 3 - network/cache (2)

Honza, does this look right to you?  Feel free to run by Michal as well as needed.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1001d232f8a
Remove some non-used telemetry IDs - part 1 - expired keys in nsBrowserApp, r=gfritzsche
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bf710c1a503
Remove some non-used telemetry IDs - part 2 - network/cache, r=gfritzsche
https://hg.mozilla.org/integration/mozilla-inbound/rev/3af9c0fa2ba6
Remove some non-used telemetry IDs - part 3 - random things, r=chutten
https://hg.mozilla.org/integration/mozilla-inbound/rev/27b1dd843116
Remove some non-used telemetry IDs - part 4 - random things (2), r=chutten
https://hg.mozilla.org/integration/mozilla-inbound/rev/81120708f321
Remove some non-used telemetry IDs - part 5 - Search and other stuff, r=gfritzsche

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3892088d89b
Remove some non-used telemetry IDs - part 6 - histogram-whitelists.json fixed CLOSED TREE, r=me

Comment 22

[Carsten Book [:Tomcat]]

backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=30783958&repo=mozilla-inbound

Comment 23

[Chris H-C :chutten]

Comment on attachment 8764849 [details] [diff] [review]
patch 4 - random things

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

::: toolkit/components/telemetry/histogram-whitelists.json
@@ -555,5 @@
>      "HTTP_TRANSACTION_USE_ALTSVC_OE",
> -    "IDLE_NOTIFY_BACK_LISTENERS",
> -    "IDLE_NOTIFY_BACK_MS",
> -    "IDLE_NOTIFY_IDLE_LISTENERS",
> -    "IDLE_NOTIFY_IDLE_MS",

Missed this the first time 'round. You didn't remove this hgram, but removed it from the whitelist. This is causing the bustage.

Comment 24

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3531fd033083
Backed out changeset d3892088d89b 
https://hg.mozilla.org/integration/mozilla-inbound/rev/28dbd9c39d3e
Backed out changeset 81120708f321 
https://hg.mozilla.org/integration/mozilla-inbound/rev/1704aab64d91
Backed out changeset 27b1dd843116 
https://hg.mozilla.org/integration/mozilla-inbound/rev/5297b24d7720
Backed out changeset 3af9c0fa2ba6 
https://hg.mozilla.org/integration/mozilla-inbound/rev/03b169a1cd5c
Backed out changeset 4bf710c1a503 
https://hg.mozilla.org/integration/mozilla-inbound/rev/c51fe95aa69a
Backed out changeset e1001d232f8a for bustage on a CLOSED TREE

Comment 25

[Honza Bambas (:mayhemer)]

Comment on attachment 8764848 [details] [diff] [review]
patch 3 - network/cache (2)

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

::: widget/nsIdleService.cpp
@@ -542,5 @@
>      return NS_OK;
>    }
>  
>    // Mark all idle services as non-idle, and calculate the next idle timeout.
> -  Telemetry::AutoTimer<Telemetry::IDLE_NOTIFY_BACK_MS> timer;

I don't see these removed from the Histograms.json (and other) files

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b022cc28da9b
Remove some non-used telemetry IDs - part 1 - expired keys in nsBrowserApp, r=gfritzsche
https://hg.mozilla.org/integration/mozilla-inbound/rev/33d663a3d351
Remove some non-used telemetry IDs - part 2 - network/cache, r=gfritzsche
https://hg.mozilla.org/integration/mozilla-inbound/rev/dff93c4f631d
Remove some non-used telemetry IDs - part 3 - random things, r=chutten
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ac748f4d677
Remove some non-used telemetry IDs - part 4 - random things (2), r=chutten
https://hg.mozilla.org/integration/mozilla-inbound/rev/3896ec4f3a39
Remove some non-used telemetry IDs - part 5 - Search and other stuff, r=gfritzsche

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3c807b23268
Remove some non-used telemetry IDs - part 6 - wrong IDs removed - CLOSED TREE, r=me

Comment 28

[Andrea Marchesini [:baku]]

Attachment: patch 3 - network/cache (2)

Comment 29

[Andrea Marchesini [:baku]]

> Missed this the first time 'round. You didn't remove this hgram, but removed
> it from the whitelist. This is causing the bustage.

Yeah. I realized it by myself. what happened was that half of the patch 3 went into patch 4. I dream an automatic 'hg qref'.
I pushed a fix and a new version of patch 3.

Comment 30

[Honza Bambas (:mayhemer)]

Comment on attachment 8766184 [details] [diff] [review]
patch 3 - network/cache (2)

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

::: netwerk/cache/nsDiskCacheMap.cpp
@@ -1415,5 @@
>          // would help.
>      }
>  
> -    // We want this after the lock to prove that flushing a file isn't that expensive
> -    Telemetry::AutoTimer<Telemetry::NETWORK_DISK_CACHE_REVALIDATION> totalTimer;

I don't see this removed from histograms.json

::: netwerk/cache/nsDiskCacheStreams.cpp
@@ -334,5 @@
> -    mozilla::Telemetry::ID id;
> -    if (NS_IsMainThread())
> -        id = mozilla::Telemetry::NETWORK_DISK_CACHE_STREAMIO_CLOSE_MAIN_THREAD;
> -    else
> -        id = mozilla::Telemetry::NETWORK_DISK_CACHE_STREAMIO_CLOSE;

I don't see these removed from histograms.json

::: widget/nsIdleService.cpp
@@ -542,5 @@
>      return NS_OK;
>    }
>  
>    // Mark all idle services as non-idle, and calculate the next idle timeout.
> -  Telemetry::AutoTimer<Telemetry::IDLE_NOTIFY_BACK_MS> timer;

if this is a "network/cache" patch, then these probes don't belong here.

you remove them from histogram-whitelists.json in this patch but not from Histograms.json.  move to it's own patch.

Comment 31

[Andrea Marchesini [:baku]]

Attachment: patch 3 - network/cache (2)

Comment 32

[Andrea Marchesini [:baku]]

Attachment: patch 7 - widget

Comment 33

[Honza Bambas (:mayhemer)]

Comment on attachment 8766236 [details] [diff] [review]
patch 3 - network/cache (2)

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

thanks!  now it's correct.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ed188787902
Remove some non-used telemetry IDs - part 7 - network/cache (2), r=mayhemer

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b022cc28da9b
https://hg.mozilla.org/mozilla-central/rev/33d663a3d351
https://hg.mozilla.org/mozilla-central/rev/dff93c4f631d
https://hg.mozilla.org/mozilla-central/rev/0ac748f4d677
https://hg.mozilla.org/mozilla-central/rev/3896ec4f3a39
https://hg.mozilla.org/mozilla-central/rev/f3c807b23268

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7ed188787902

Comment 37

[Georg Fritzsche [:gfritzsche]]

(In reply to Chris H-C :chutten from comment #23)
> Comment on attachment 8764849 [details] [diff] [review]
> patch 4 - random things
> 
> Review of attachment 8764849 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/histogram-whitelists.json
> @@ -555,5 @@
> >      "HTTP_TRANSACTION_USE_ALTSVC_OE",
> > -    "IDLE_NOTIFY_BACK_LISTENERS",
> > -    "IDLE_NOTIFY_BACK_MS",
> > -    "IDLE_NOTIFY_IDLE_LISTENERS",
> > -    "IDLE_NOTIFY_IDLE_MS",
> 
> Missed this the first time 'round. You didn't remove this hgram, but removed
> it from the whitelist. This is causing the bustage.

Andrea, did you clear up this r-? It looks like that patch landed?

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/86e45db70ad2
part 7 - Remove some non-used telemetry IDs - widget directory, r=gfritzsche

Comment 39

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] [away Jun 24 - Jul 3] from comment #37)
> (In reply to Chris H-C :chutten from comment #23)
> > Comment on attachment 8764849 [details] [diff] [review]
> > patch 4 - random things
> > 
> > Review of attachment 8764849 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: toolkit/components/telemetry/histogram-whitelists.json
> > @@ -555,5 @@
> > >      "HTTP_TRANSACTION_USE_ALTSVC_OE",
> > > -    "IDLE_NOTIFY_BACK_LISTENERS",
> > > -    "IDLE_NOTIFY_BACK_MS",
> > > -    "IDLE_NOTIFY_IDLE_LISTENERS",
> > > -    "IDLE_NOTIFY_IDLE_MS",
> > 
> > Missed this the first time 'round. You didn't remove this hgram, but removed
> > it from the whitelist. This is causing the bustage.
> 
> Andrea, did you clear up this r-? It looks like that patch landed?

Comment 40

[Andrea Marchesini [:baku]]

> > Andrea, did you clear up this r-? It looks like that patch landed?

At the beginning it was a r+. The I make a mess with the ordering of the patches and it was backed out. In the meantime I fixed the issue and I landed it again. This r- was in between the back-out and the rest. So yes, the patch is landed. If you want to take a look the landed patch is:

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ac748f4d677

Comment 41

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/86e45db70ad2