1465052 - Instrument pending operations list high-water mark spilling

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[Jan-Erik Rediger [:janerik]]

After bug 1454606 we need to instrument the high-water mark spilling of the pending operations list and record when it is actually hit (the current value of 10000 operations was copied from the IPC code).

This requires adding a new probe and a recording when the high water mark is handled.
Care needs to be taken to actually apply that operation after the operations are applied and recording is enabled again to avoid re-entrance.

Comment 1

[Jan-Erik Rediger [:janerik]]

Attachment: Bug 1465052 - Record when the high-water mark for pending operations is hit.

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

Comment 2

[Jan-Erik Rediger [:janerik]]

Attachment: data-review-request-1465052.txt

Comment 3

[Alessio Placitelli [:Dexter]]

Comment on attachment 8983016 [details]
Bug 1465052 - Record when the high-water mark for pending operations is hit.

https://reviewboard.mozilla.org/r/248876/#review255406

::: toolkit/components/telemetry/TelemetryScalar.cpp:1273
(Diff revision 1)
>  }
>  
>  void internal_ApplyPendingOperations(const StaticMutexAutoLock& lock);
>  
>  /**
> + * Record that the high-water mark for the pending operations list was reached once.

Ugh, I don't like this, but looks we don't have many options in this case.

We could probably propagate the "pending operations overflow" error up in the call stack and then rely on the usual API for changing the value. It seems tricky on a first look, but might be worth checking into.

If that's too much of a problem, we can stick back on this.

Comment 4

[Alessio Placitelli [:Dexter]]

Comment on attachment 8983016 [details]
Bug 1465052 - Record when the high-water mark for pending operations is hit.

https://reviewboard.mozilla.org/r/248876/#review255412

::: toolkit/components/telemetry/TelemetryScalar.cpp:1273
(Diff revision 1)
>  }
>  
>  void internal_ApplyPendingOperations(const StaticMutexAutoLock& lock);
>  
>  /**
> + * Record that the high-water mark for the pending operations list was reached once.

I don't like this -> I don't like using Scalar API internals for setting values.

Comment 5

[Jan-Erik Rediger [:janerik]]

Comment on attachment 8983016 [details]
Bug 1465052 - Record when the high-water mark for pending operations is hit.

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

Comment 6

[Alessio Placitelli [:Dexter]]

Comment on attachment 8983016 [details]
Bug 1465052 - Record when the high-water mark for pending operations is hit.

https://reviewboard.mozilla.org/r/248876/#review255452

::: toolkit/components/telemetry/TelemetryScalar.cpp:1271
(Diff revisions 1 - 2)
>    *aRet = scalar;
>    return NS_OK;
>  }
>  
>  void internal_ApplyPendingOperations(const StaticMutexAutoLock& lock);
> +void internal_RecordScalarAction(const StaticMutexAutoLock& lock,

nit: we don't need to forward declare this anymore, since we're not using it below

Comment 7

[Jan-Erik Rediger [:janerik]]

Comment on attachment 8983016 [details]
Bug 1465052 - Record when the high-water mark for pending operations is hit.

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

Comment 8

[Chris H-C :chutten]

Comment on attachment 8983017 [details]
data-review-request-1465052.txt

DATA COLLECTION REVIEW RESPONSE:

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

Standard Telemetry mechanisms apply.

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

Standard Telemetry mechanisms apply.

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

N/A

    Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under? **

Category 1, Technical

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

Default-on

    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

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

Yes

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

Yes. :janerik, I expect you'll take care of filing any follow-ups for analysis and renewal/expiry.

---
Result: datareview+

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e4f7fc8dc38c
Record when the high-water mark for pending operations is hit. r=Dexter

Comment 10

[Arthur Iakab [arthur_iakab]]

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