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

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, enhancement, P2)

Description

[Tom Tung [:tt, :ttung]]

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.

Comment 1

[Tom Tung [:tt, :ttung]]

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

Comment 2

[Jan Varga [:janv]]

(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.

Comment 3

[Tom Tung [:tt, :ttung]]

Attachment: telemetry.patch

Comment 4

[Tom Tung [:tt, :ttung]]

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.

Comment 5

[Jan Varga [:janv]]

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

Comment 6

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1481716: Add telemetry probs to track how long do we take for initializing repositories and the successful rate. r=janv

Comment 7

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1481716: Add telemetry probes to track how long do we take for initializing repositories and the successful rate. r=janv

Addressed comment 5

Comment 8

[Tom Tung [:tt, :ttung]]

Attachment: data-request.txt

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!

Comment 9

[François Marier [: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": ["[e-mail]"],
> +    "description": "Boolean for indicate whether initialization of QuotaManager's defualt repository succeed or not."

Typo: default

Comment 10

[François Marier [:francois]]

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.

Comment 11

[Tom Tung [:tt, :ttung]]

(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.

Comment 12

[Tom Tung [:tt, :ttung]]

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

Comment 13

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1481716: Add telemetry probes to track how long do we take for initializing repositories and the successful rate. r=janv

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.

Comment 14

[Tom Tung [:tt, :ttung]]

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

Comment 15

[Jan Varga [:janv]]

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

[Jan Varga [:janv]]

(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

Comment 17

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 18

[Jan Varga [:janv]]

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

Comment 19

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1481716: Add telemetry probes to track how long do we take for initializing repositories and the successful rate; r=janv

Addressed comment

Comment 20

[Tom Tung [:tt, :ttung]]

Attachment: 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!

Comment 21

[Tom Tung [:tt, :ttung]]

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.

Comment 22

[Jan Varga [:janv]]

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 ?

Comment 23

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1481716: Add telemetry probes to track how long do we take for initializing repositories and the successful rate; r=janv

Comment 24

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1481716: Add telemetry probes to track how long do we take for initializing repositories and the successful rate; r=janv

Comment 25

[Tom Tung [:tt, :ttung]]

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.

Comment 26

[François Marier [:francois]]

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.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
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

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/daed617054ce