1359708 - Add a telemetry probe for Storage API features

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, enhancement)

Description

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Add a telemetry probe for Storage API features

Comment 1

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1359708 - Add a telemetry probe for Storage API features, data-r=bsmedberg,

Review commit: https://reviewboard.mozilla.org/r/134324/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/134324/

Comment 2

[Jan Varga [:janv]]

Comment on attachment 8862373 [details]
Bug 1359708 - Add a telemetry probe for Storage API features, data-r=bsmedberg,

https://reviewboard.mozilla.org/r/134324/#review137254

::: dom/quota/StorageManager.cpp:842
(Diff revision 1)
>  already_AddRefed<Promise>
>  StorageManager::Persist(ErrorResult& aRv)
>  {
>    MOZ_ASSERT(mOwner);
>  
> +  Telemetry::ScalarAdd(Telemetry::ScalarID::NAVIGATOR_STORAGE_PERSIST_COUNT, 1);

Nit: max 80 chars per line

::: toolkit/components/telemetry/Scalars.yaml:609
(Diff revision 1)
> +    notification_emails:
> +      - [e-mail]
> +      - [e-mail]
> +    release_channel_collection: opt-in
> +    record_in_processes:
> +      - 'main'

Is this correct ? Do you really want it only for main process ?
ScalarAdd is called in StorageManager and that's the client/child side of IPC communication. So these telemetry probes would only work when FF runs in non-e10s mode.
But maybe I'm missing something.

Comment 3

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8862373 [details]
Bug 1359708 - Add a telemetry probe for Storage API features, data-r=bsmedberg,

https://reviewboard.mozilla.org/r/134324/#review137254

> Nit: max 80 chars per line

But the last char is exactly the 80th char. Shall I move the second parameter to the next line here?

> Is this correct ? Do you really want it only for main process ?
> ScalarAdd is called in StorageManager and that's the client/child side of IPC communication. So these telemetry probes would only work when FF runs in non-e10s mode.
> But maybe I'm missing something.

I was wrong. Thanks for the correction.

Comment 4

[Jan Varga [:janv]]

Comment on attachment 8862373 [details]
Bug 1359708 - Add a telemetry probe for Storage API features, data-r=bsmedberg,

https://reviewboard.mozilla.org/r/134324/#review137298

::: dom/quota/StorageManager.cpp:842
(Diff revision 1)
>  already_AddRefed<Promise>
>  StorageManager::Persist(ErrorResult& aRv)
>  {
>    MOZ_ASSERT(mOwner);
>  
> +  Telemetry::ScalarAdd(Telemetry::ScalarID::NAVIGATOR_STORAGE_PERSIST_COUNT, 1);

Ah, I was confused by the second ScalarAdd, why it that different ?

Comment 5

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8862373 [details]
Bug 1359708 - Add a telemetry probe for Storage API features, data-r=bsmedberg,

https://reviewboard.mozilla.org/r/134324/#review137298

> Ah, I was confused by the second ScalarAdd, why it that different ?

PERSIST 7 chars, ESTIMATE 8 chars.

Comment 6

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8862373 [details]
Bug 1359708 - Add a telemetry probe for Storage API features, data-r=bsmedberg,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134324/diff/1-2/

Comment 7

[Jan Varga [:janv]]

Comment on attachment 8862373 [details]
Bug 1359708 - Add a telemetry probe for Storage API features, data-r=bsmedberg,

https://reviewboard.mozilla.org/r/134324/#review137298

> PERSIST 7 chars, ESTIMATE 8 chars.

Amazing!

Sorry, never mind, I'm doing too many things at once and this new review UI doesn't help me much.

Comment 8

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8862373 [details]
Bug 1359708 - Add a telemetry probe for Storage API features, data-r=bsmedberg,

https://reviewboard.mozilla.org/r/134324/#review137298

> Amazing!
> 
> Sorry, never mind, I'm doing too many things at once and this new review UI doesn't help me much.

I understood the pain here. I consider the other reviewer may request to use mozreview, so I switch to mozreview this time.

Comment 9

[Benjamin Smedberg]

Comment on attachment 8862373 [details]
Bug 1359708 - Add a telemetry probe for Storage API features, data-r=bsmedberg,

https://reviewboard.mozilla.org/r/134324/#review138092

data-r=me

Comment 10

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8862373 [details]
Bug 1359708 - Add a telemetry probe for Storage API features, data-r=bsmedberg,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134324/diff/2-3/

Comment 11

[Jan Varga [:janv]]

Comment on attachment 8862373 [details]
Bug 1359708 - Add a telemetry probe for Storage API features, data-r=bsmedberg,

https://reviewboard.mozilla.org/r/134324/#review138484

Comment 12

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8862373 [details]
Bug 1359708 - Add a telemetry probe for Storage API features, data-r=bsmedberg,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134324/diff/3-4/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aebffbddaafc
Add a telemetry probe for Storage API features, data-r=bsmedberg, r=bsmedberg,janv

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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