1925205 - LSNG: Add telemetry for RequestAllowToClose response time

Status: RESOLVED FIXED

(Core :: Storage: localStorage & sessionStorage, task, P3)

Description

[Jan Varga [:janv]]

It would be good to know how long it takes to allow the closing and also check the response time before and after bug 1920665.

Comment 1

[Jan Varga [:janv]]

Attachment: Bug 1925205 - LSNG: Add telemetry for RequestAllowToClose response time; r=#dom-storage

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a0ae96c2b4d4
LSNG: Add telemetry for RequestAllowToClose response time; r=dom-storage-reviewers,asuth

Comment 3

[Norisz Fay [:noriszfay]]

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

Comment 4

[Chris H-C :chutten]

This is experiencing a high rate of invalid state errors: https://sql.telemetry.mozilla.org/queries/103396/source#254740

According to docs, a timing_distribution could get invalid_state errors when:

invalid_state: If a non-existing/stopped timer is stopped again.

Does this sound like something that might be happening?

Comment 5

[Jan Varga [:janv]]

(In reply to Chris H-C :chutten from comment #4)

This is experiencing a high rate of invalid state errors: https://sql.telemetry.mozilla.org/queries/103396/source#254740

According to docs, a timing_distribution could get invalid_state errors when:

invalid_state: If a non-existing/stopped timer is stopped again.

Does this sound like something that might be happening?

I suspect this might have something to do with computers going to sleep between start and stopAndAccumulate. We actually have some helpers, and I've been planning to enhance this new metric to take advantage of them.

Could something else be stopping the timer?
Is there an API we could use to check if the timer is already stopped?

Comment 6

[Jan Varga [:janv]]

Hm, it's also possible that in some cases the IPC message AllowToClose is sent from the child without a prior IPC message RequestAllowToClose from parent and that seems more likely than the theory in comment 5. I'll try to fix this soon.

Comment 7

[Chris H-C :chutten]

(In reply to Jan Varga [:janv] from comment #5)

(In reply to Chris H-C :chutten from comment #4)

This is experiencing a high rate of invalid state errors: https://sql.telemetry.mozilla.org/queries/103396/source#254740

According to docs, a timing_distribution could get invalid_state errors when:

invalid_state: If a non-existing/stopped timer is stopped again.

Does this sound like something that might be happening?

I suspect this might have something to do with computers going to sleep between start and stopAndAccumulate. We actually have some helpers, and I've been planning to enhance this new metric to take advantage of them.

Could something else be stopping the timer?

The only way these timers are stopped are:

• You call either the stop or cancel APIs
• The user opts out of data upload (only a problem if they then opt back in before you call the stop or cancel APIs and stay opted in long enough to upload the error)

Is there an API we could use to check if the timer is already stopped?

Nope. What I was intending to do via the design of the C++ API to take the TimerID as an rvalue was to signal that the id, after std::move, was no longer safe to use. (In Rust the borrow checker does this, and it seemed like a good idea).

Alas, I got a little lazy and just typedef'd the id instead of newtype-ing it, so I didn't define the move semantics in such a way to make this a) discoverable by the instrumentor, or b) more pleasant to use.

I guess what we properly want is an opaque struct TimerId that forbids copies and invalidates itself on move (by setting itself to 0 which is reserved). I've now finally filed bug 1931927 for that idea.

Comment 8

[Jan Varga [:janv]]

The problem with invalid state errors is being fixed in https://phabricator.services.mozilla.com/D234828 based on the current options.