Remove some non-used telemetry IDs

RESOLVED FIXED in Firefox 50

Status

()

defect
P4
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: baku, Assigned: baku)

Tracking

Trunk
mozilla50
Other
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: [measurement:client])

Attachments

(7 attachments, 6 obsolete attachments)

8.49 KB, patch
gfritzsche
: review+
Details | Diff | Splinter Review
12.75 KB, patch
gfritzsche
: review+
Details | Diff | Splinter Review
9.71 KB, patch
gfritzsche
: review+
Details | Diff | Splinter Review
10.59 KB, patch
chutten
: review-
Details | Diff | Splinter Review
29.11 KB, patch
chutten
: review+
Details | Diff | Splinter Review
17.61 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
5.64 KB, patch
gfritzsche
: review+
Details | Diff | Splinter Review
Assignee

Description

3 years ago
No description provided.
Assignee

Comment 1

3 years ago
Attachment #8764588 - Flags: review?(gfritzsche)
Assignee

Comment 2

3 years ago
Attachment #8764589 - Flags: review?(gfritzsche)
Assignee

Comment 3

3 years ago
Posted patch part 3 - network/cache (2) (obsolete) — Splinter Review
Attachment #8764590 - Flags: review?(gfritzsche)
Assignee

Comment 4

3 years ago
Posted patch patch 3 - network/cache (2) (obsolete) — Splinter Review
Attachment #8764590 - Attachment is obsolete: true
Attachment #8764590 - Flags: review?(gfritzsche)
Attachment #8764591 - Flags: review?(gfritzsche)
Assignee

Comment 5

3 years ago
Posted patch patch 4 - random things (obsolete) — Splinter Review
Attachment #8764603 - Flags: review?(gfritzsche)
Assignee

Comment 6

3 years ago
Posted patch patch 5 - random things (2) (obsolete) — Splinter Review
Attachment #8764615 - Flags: review?(gfritzsche)
Assignee

Comment 7

3 years ago
Attachment #8764619 - Flags: review?(gfritzsche)
Attachment #8764588 - Flags: review?(gfritzsche) → review+
Attachment #8764589 - Flags: review?(gfritzsche) → review+
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 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 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.
Attachment #8764619 - Flags: review?(gfritzsche) → review+
Attachment #8764591 - Flags: review?(gfritzsche) → review?(chutten)
Attachment #8764603 - Flags: review?(gfritzsche) → review?(chutten)
Attachment #8764615 - Flags: review?(gfritzsche) → review?(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
Attachment #8764591 - Flags: review?(chutten) → review-
Comment on attachment 8764603 [details] [diff] [review]
patch 4 - random things

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

Remove probes from histograms_whitelist
Attachment #8764603 - Flags: review?(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() ?
Attachment #8764615 - Flags: review?(chutten) → review-
Assignee

Updated

3 years ago
Depends on: 1156565
Blocks: 1136118
No longer depends on: 1156565
Assignee

Comment 14

3 years ago
Posted patch patch 3 - network/cache (2) (obsolete) — Splinter Review
Attachment #8764591 - Attachment is obsolete: true
Attachment #8764848 - Flags: review?(jduell.mcbugs)
Assignee

Comment 15

3 years ago
Attachment #8764603 - Attachment is obsolete: true
Attachment #8764849 - Flags: review?(chutten)
Assignee

Comment 16

3 years ago
Attachment #8764615 - Attachment is obsolete: true
Attachment #8764855 - Flags: review?(chutten)
Assignee

Updated

3 years ago
Blocks: 1282025
Comment on attachment 8764849 [details] [diff] [review]
patch 4 - random things

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

LGTM
Attachment #8764849 - Flags: review?(chutten) → review+
Comment on attachment 8764855 [details] [diff] [review]
patch 5 - random things (2)

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

LGTM
Attachment #8764855 - Flags: review?(chutten) → review+

Comment 19

3 years ago
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.
Attachment #8764848 - Flags: review?(jduell.mcbugs) → review?(honzab.moz)
Assignee

Updated

3 years ago
Keywords: leave-open

Comment 20

3 years ago
Pushed by amarchesini@mozilla.com:
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

3 years ago
Pushed by amarchesini@mozilla.com:
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 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.
Attachment #8764849 - Flags: review+ → review-
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

3 years ago
Pushed by amarchesini@mozilla.com:
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

3 years ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3c807b23268
Remove some non-used telemetry IDs - part 6 - wrong IDs removed - CLOSED TREE, r=me
Assignee

Comment 28

3 years ago
Posted patch patch 3 - network/cache (2) (obsolete) — Splinter Review
Attachment #8764848 - Attachment is obsolete: true
Attachment #8764848 - Flags: review?(honzab.moz)
Flags: needinfo?(amarchesini)
Attachment #8766184 - Flags: review?(honzab.moz)
Assignee

Comment 29

3 years ago
> 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 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.
Attachment #8766184 - Flags: review?(honzab.moz) → review-
Assignee

Comment 31

3 years ago
Attachment #8766184 - Attachment is obsolete: true
Attachment #8766236 - Flags: review?(honzab.moz)
Assignee

Comment 32

3 years ago
Attachment #8766237 - Flags: review?(gfritzsche)
Comment on attachment 8766236 [details] [diff] [review]
patch 3 - network/cache (2)

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

thanks!  now it's correct.
Attachment #8766236 - Flags: review?(honzab.moz) → review+

Comment 34

3 years ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ed188787902
Remove some non-used telemetry IDs - part 7 - network/cache (2), r=mayhemer
Depends on: 1283211

Updated

3 years ago
See Also: → 1284035
Attachment #8766237 - Flags: review?(gfritzsche) → review+
(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?
OS: Unspecified → All
Priority: -- → P4
Hardware: Unspecified → Other
Whiteboard: [measurement:client]
Version: unspecified → Trunk

Comment 38

3 years ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/86e45db70ad2
part 7 - Remove some non-used telemetry IDs - widget directory, r=gfritzsche
Assignee

Updated

3 years ago
Keywords: leave-open
(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?
Flags: needinfo?(amarchesini)
Assignee

Comment 40

3 years ago
> > 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
Flags: needinfo?(amarchesini)

Comment 41

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/86e45db70ad2
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.