1135408 - Add telemetry reporting for device resets

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Bas Schouten (:bas.schouten)]

Attachment: Add telemetry reporting for device resets

We should record device removals occurring out in the wild so we can see when we're impacting them in a negative way, this will be particularly important when we no longer crash on them.

Comment 1

[Vladan Djeric (:vladan)]

Comment on attachment 8567557 [details] [diff] [review]
Add telemetry reporting for device resets

Review of attachment 8567557 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +476,5 @@
>   */
>      bool didReset = false;
> +    DeviceResetReason resetReason = DeviceResetReason::OK;
> +    if (DidRenderingDeviceReset(&resetReason)) {
> +      Telemetry::Accumulate(Telemetry::DEVICE_RESET_REASON, uint32_t(resetReason));

you're never going to report DeviceResetReason::OK, is that intended?

@@ +1175,5 @@
>        return true;
>      }
>    }
>    if (mD3D11ContentDevice) {
>      if (mD3D11ContentDevice->GetDeviceRemovedReason() != S_OK) {

You don't want to report the other device types resetting?

::: toolkit/components/telemetry/Histograms.json
@@ +212,5 @@
>    },
> +  "DEVICE_RESET_REASON": {
> +    "expires_in_version": "never",
> +    "kind": "enumerated",
> +    "n_values": 5,

- There are 6 enum values though
- Also, you should round up n_values to 10 or more to leave room for future values. It's not really possible to change the definition of a histogram after data has been collected

Comment 2

[Vladan Djeric (:vladan)]

Comment on attachment 8567557 [details] [diff] [review]
Add telemetry reporting for device resets

Review of attachment 8567557 [details] [diff] [review]:
-----------------------------------------------------------------

After talking to Bas in person, r+ if you change n_values to 10

Comment 3

[Carsten Book [:Tomcat]]

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

Comment 4

[Bas Schouten (:bas.schouten)]

Comment on attachment 8567557 [details] [diff] [review]
Add telemetry reporting for device resets

Approval Request Comment
[Feature/regressing bug #]: None
[User impact if declined]: Unable to tell TDRs in the wild
[Describe test coverage new/current, TreeHerder]: Limited
[Risks and why]: Very low, only telemetry reporting
[String/UUID change made/needed]: None

Comment 5

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8567557 [details] [diff] [review]
Add telemetry reporting for device resets

Although this change has only been on m-c for a few hours, I spoke with Vladan and he said this is very low risk. We also really need to get more graphics data to get on top of some of the issues that we're seeing. 37 is now Beta so I'm approving for Beta 1.

Beta+

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/54db49df50e0