Add probes to track how long does it take for the QM to initialize the whole storage directory

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P2
normal
RESOLVED FIXED
10 months ago
7 months ago

People

(Reporter: tt, Assigned: tt)

Tracking

(Blocks 1 bug)

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(2 attachments, 7 obsolete attachments)

2.28 KB, text/plain
francois
: review+
Details
3.18 KB, patch
janv
: review+
Details | Diff | Splinter Review
Assignee

Description

10 months ago
Based on bug 1422815 comment 32 and bug 1422815 comment 33, we want to move the content of metadata files to storage.sqlite. Before doing that, it's better to have somethings to examine how much do we improve. Therefore, this bug is filed for adding probes to track the initialization time for sweeping the whole storage directory.
Assignee

Comment 1

10 months ago
Hi Jan,

I want to insert a few telemetry probes to measure how long do we take for initializing storage so that we could compare. So basically, I want to track how long does it take for the function |EnsureOriginIsInitialized()| in general. To do this, I'm thinking to add them at [1], [2], [3], [4], [5], and [6]. Do you have any suggestion about that?

Measuring the storage initialization time
[1] https://searchfox.org/mozilla-central/rev/f0c15db995198a1013e1c5f5b5bea54ef83f1049/dom/quota/ActorsParent.cpp#5051
[2] https://searchfox.org/mozilla-central/rev/f0c15db995198a1013e1c5f5b5bea54ef83f1049/dom/quota/ActorsParent.cpp#5217
Measuring the storage version upgrading time
[3] https://searchfox.org/mozilla-central/rev/f0c15db995198a1013e1c5f5b5bea54ef83f1049/dom/quota/ActorsParent.cpp#5175
[4] https://searchfox.org/mozilla-central/rev/f0c15db995198a1013e1c5f5b5bea54ef83f1049/dom/quota/ActorsParent.cpp#5201
Measuring the repository initialization time
[5] https://searchfox.org/mozilla-central/rev/f0c15db995198a1013e1c5f5b5bea54ef83f1049/dom/quota/ActorsParent.cpp#5439
[6] https://searchfox.org/mozilla-central/rev/f0c15db995198a1013e1c5f5b5bea54ef83f1049/dom/quota/ActorsParent.cpp#5455
Flags: needinfo?(jvarga)

Comment 2

10 months ago
(In reply to Tom Tung [:tt] from comment #1)
> Measuring the storage initialization time

Ok, this is be total time required to initialize storage.
Line numbers look correct.

> Measuring the storage version upgrading time
I don't know, this would measure total time of running upgrade functions.
Sometimes it can be just one function, sometimes all 3 functions.
I think it would be better to know each time individually too.
Line numbers look correct.

> Measuring the repository initialization time
Yeah, this is what Andrew proposed to measure. It might be better to measure
each repository separately too.
Flags: needinfo?(jvarga)
Assignee

Comment 3

10 months ago
Posted patch telemetry.patch (obsolete) — Splinter Review
Assignee

Comment 4

10 months ago
Comment on attachment 8999519 [details] [diff] [review]
telemetry.patch

Hi Jan,

Could you help me to check these telemetries are putting in the right place and matching current coding style especially RAII method I use for QM_REPOSITORIES_INITIALIZATION_SUCCEEDED? Thanks!

I'll request for a data-review if they are fine.
Attachment #8999519 - Flags: review?(jvarga)

Comment 5

10 months ago
Comment on attachment 8999519 [details] [diff] [review]
telemetry.patch

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

I changed my mind, we should just do what :asuth proposed in bug 1422815 comment 33
Attachment #8999519 - Flags: review?(jvarga)
Priority: -- → P3

Updated

10 months ago
Blocks: 1482662
Assignee

Comment 8

9 months ago
Posted file data-request.txt (obsolete) —
Hi François,

We'd like to know how long does it take for sweeping default and temporary repositories, and the success rate of that so that later we can have a baseline to see how much do we improve. 

Could you help us to have a data-review? Thanks!
Attachment #8999845 - Flags: review?(francois)
Comment on attachment 8999843 [details] [diff] [review]
Bug 1481716: Add telemetry probes to track how long do we take for initializing repositories and the successful rate. r=janv

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

::: toolkit/components/telemetry/Histograms.json
@@ +13926,5 @@
> +    "bug_numbers": [1481716],
> +    "kind": "boolean",
> +    "releaseChannelCollection": "opt-out",
> +    "alert_emails": ["ttung@mozilla.com"],
> +    "description": "Boolean for indicate whether initialization of QuotaManager's defualt repository succeed or not."

Typo: default
Comment on attachment 8999845 [details]
data-request.txt

1) Is there or will there be **documentation** that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes, in Histograms.json.

2) Is there a control mechanism that allows the user to turn the data collection on and off?

Yes, telemetry setting.

3) If the request is for permanent data collection, is there someone who will monitor the data over time?**

Not permanent.

4) Using the **[category system of data types](https://wiki.mozilla.org/Firefox/Data_Collection)** on the Mozilla wiki, what collection type of data do the requested measurements fall under?  **

Category 1.

5) Is the data collection request for default-on or default-off?

Default ON, all channels.

6) Does the instrumentation include the addition of **any *new* identifiers** (whether anonymous or otherwise; e.g., username, random IDs, etc.  See the appendix for more details)?

No.

7) Is the data collection covered by the existing Firefox privacy notice?

Yes.

8) Does there need to be a check-in in the future to determine whether to renew the data?

No, telemetry alerts are fine.
Attachment #8999845 - Flags: review?(francois) → review+
Assignee

Comment 11

9 months ago
(In reply to François Marier [:francois] from comment #9)
> > +    "description": "Boolean for indicate whether initialization of QuotaManager's defualt repository succeed or not."
> 
> Typo: default

Thanks for the data-review and the correction! Will correct that.
Assignee

Updated

9 months ago
Blocks: 1436188
Assignee

Comment 12

9 months ago
Comment on attachment 8999843 [details] [diff] [review]
Bug 1481716: Add telemetry probes to track how long do we take for initializing repositories and the successful rate. r=janv

Drop the request here per bug 1436188 comment #12
Attachment #8999843 - Flags: review?(jvarga)
Assignee

Comment 13

9 months ago
IIUC, we decided to track the initialization success rate like (num_of_success_dir/num_of_dir_under_a_respository  per cache, or idb). I would like to move that to bug 1436188.

Update patch for removing boolean probes.
Attachment #8999843 - Attachment is obsolete: true
Attachment #8999845 - Attachment is obsolete: true
Attachment #9005625 - Flags: review?(jvarga)
Assignee

Comment 14

9 months ago
(In reply to Tom Tung [:tt] from comment #13)
I'll resend a data-review after the patch gets r+

Updated

8 months ago
Priority: P3 → P2

Comment 15

8 months ago
Comment on attachment 9005625 [details] [diff] [review]
Bug 1481716: Add telemetry probes to track how long do we take for initializing repositories and the successful rate. r=janv

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

::: toolkit/components/telemetry/Histograms.json
@@ +14044,5 @@
> +    "record_in_processes": ["main"],
> +    "expires_in_version": "68",
> +    "bug_numbers": [1481716],
> +    "kind": "exponential",
> +    "high": 5000,

Hm 5 seconds, that looks too low. There might be some extreme cases which we want to catch, right ?

Comment 16

8 months ago
(In reply to Jan Varga [:janv] from comment #15)
> Comment on attachment 9005625 [details] [diff] [review]
> Bug 1481716: Add telemetry probes to track how long do we take for
> initializing repositories and the successful rate. r=janv
> 
> Review of attachment 9005625 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/Histograms.json
> @@ +14044,5 @@
> > +    "record_in_processes": ["main"],
> > +    "expires_in_version": "68",
> > +    "bug_numbers": [1481716],
> > +    "kind": "exponential",
> > +    "high": 5000,
> 
> Hm 5 seconds, that looks too low. There might be some extreme cases which we
> want to catch, right ?

Andrew, in bug 1436188 comment 11, you said 5 secs is enough. However, I still think we should take this opportunity and catch possible extreme cases. It's an exponential histogram, so extreme values won't mess up the range of standard buckets.
I propose:
"high": 30000
"n_buckets": 30
Flags: needinfo?(bugmail)
Higher is fine with me.  I thought 5 seconds was a little low, but at the same time, 5 seconds is too long for QuotaManager to take to initialize.  5 seconds is fine for an actionable threshold, but indeed having more granularity does provide additional information.
Flags: needinfo?(bugmail)

Comment 18

8 months ago
Comment on attachment 9005625 [details] [diff] [review]
Bug 1481716: Add telemetry probes to track how long do we take for initializing repositories and the successful rate. r=janv

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

::: toolkit/components/telemetry/Histograms.json
@@ +14045,5 @@
> +    "expires_in_version": "68",
> +    "bug_numbers": [1481716],
> +    "kind": "exponential",
> +    "high": 5000,
> +    "n_buckets": 25,

r=me if you change it to:
"high": 30000
"n_buckets": 30
Attachment #9005625 - Flags: review?(jvarga)
Assignee

Updated

8 months ago
Attachment #9016685 - Attachment description: : Bug 1481716: Add telemetry probes to track how long do we take for initializing repositories and the successful rate; r=janv → Bug 1481716: Add telemetry probes to track how long do we take for initializing repositories and the successful rate; r=janv
Assignee

Comment 20

8 months ago
Posted file data-request.txt
Hi François,

Although you have already done the data review in this issue, we changed what we want to collect here.

Now, we only what to know how long does it take for a user to initialize their default and temporary repositories. Also, we increased the size of buckets and the high in the histogram.

Could you have a data-review again to ensure it's still fine to do that? Thanks!
Attachment #9016687 - Flags: review?(francois)
Assignee

Comment 21

8 months ago
Jan, you said r=me if I change parameters in Histograms in comment 18, but you canceled the review there. I'm not really sure whether you think it's r+ or not, do you want to have a look at it again?

ni in the case of that the patch is still not okay.
Flags: needinfo?(jvarga)

Comment 22

8 months ago
Comment on attachment 9016685 [details] [diff] [review]
Bug 1481716: Add telemetry probes to track how long do we take for initializing repositories and the successful rate; r=janv

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

I wanted to see a patch for landing before giving r=me

::: dom/quota/ActorsParent.cpp
@@ +5435,5 @@
>  
> +  Telemetry::AccumulateTimeDelta(
> +    Telemetry::QM_REPOSITORIES_INITIALIZATION_TIME,
> +    startTime,
> +  TimeStamp::Now());

Why no indentation here ?

What about this:
 Telemetry::AccumulateTimeDelta(Telemetry::QM_REPOSITORIES_INITIALIZATION_TIME,
                                startTime,
	                        TimeStamp::Now());

::: toolkit/components/telemetry/Histograms.json
@@ +14311,5 @@
>      "description": "How long the AudioContext would become audible since it was created, time unit is seconds.",
>      "releaseChannelCollection": "opt-out"
> +  },
> +  "QM_REPOSITORIES_INITIALIZATION_TIME": {
> +        "record_in_processes": ["main"],

Why 6 spaces for the indentation ?

Updated

8 months ago
Flags: needinfo?(jvarga)
Assignee

Comment 25

8 months ago
Comment on attachment 9016719 [details] [diff] [review]
Bug 1481716: Add telemetry probes to track how long do we take for initializing repositories and the successful rate; r=janv

Thanks for pointing them out. I should have checked indentation before uploading the patch.
Attachment #9016719 - Flags: review?(jvarga)

Updated

8 months ago
Attachment #9016719 - Flags: review?(jvarga) → review+
Comment on attachment 9016687 [details]
data-request.txt

1) Is there or will there be **documentation** that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes, in Histograms.json.

2) Is there a control mechanism that allows the user to turn the data collection on and off?

Yes, telemetry setting.

3) If the request is for permanent data collection, is there someone who will monitor the data over time?**

Not permanent.

4) Using the **[category system of data types](https://wiki.mozilla.org/Firefox/Data_Collection)** on the Mozilla wiki, what collection type of data do the requested measurements fall under?  **

Category 1.

5) Is the data collection request for default-on or default-off?

Default ON, all channels.

6) Does the instrumentation include the addition of **any *new* identifiers** (whether anonymous or otherwise; e.g., username, random IDs, etc.  See the appendix for more details)?

No.

7) Is the data collection covered by the existing Firefox privacy notice?

Yes.

8) Does there need to be a check-in in the future to determine whether to renew the data?

No, telemetry alerts are fine.
Attachment #9016687 - Flags: review?(francois) → review+

Comment 27

7 months ago
Pushed by shes050117@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/daed617054ce
Add a telemetry probe to track how long does the QM take for initializing repositories; r=janv, data-review=francois

Comment 28

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/daed617054ce
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.