Add a telemetry probe for Storage API features

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: shawnjohnjr, Assigned: shawnjohnjr)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

Add a telemetry probe for Storage API features
Assignee: nobody → shuang
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:
> +      - shuang@mozilla.com
> +      - ttung@mozilla.com
> +    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 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 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 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 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 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 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
Attachment #8862373 - Flags: review?(benjamin) → review+
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
Attachment #8862373 - Flags: review?(jvarga) → review+
Pushed by shuang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aebffbddaafc
Add a telemetry probe for Storage API features, data-r=bsmedberg, r=bsmedberg,janv
https://hg.mozilla.org/mozilla-central/rev/aebffbddaafc
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.