Closed Bug 1135408 Opened 5 years ago Closed 5 years ago

Add telemetry reporting for device resets

Categories

(Core :: Graphics, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: bas.schouten, Assigned: bas.schouten)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

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.
Attachment #8567557 - Flags: review?(vdjeric)
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
Attachment #8567557 - Flags: review?(vdjeric)
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
Attachment #8567557 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b0982cda7bb6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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
Attachment #8567557 - Flags: approval-mozilla-aurora?
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+
Attachment #8567557 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
Depends on: 1140231
Blocks: 1144122
You need to log in before you can comment on or make changes to this bug.