Closed Bug 1332511 Opened 3 years ago Closed 3 years ago

Adjust histograms for TIME_TO_FIRST_(Interaction) probes


(Core :: General, defect)

Not set



Tracking Status
firefox54 --- fixed


(Reporter: Harald, Assigned: wchen)




(2 files)

Discussed in #perf a bit what to do about the overflowing last buckets in all histograms


1. The last bucket to 100s is taking in all the overflowing values
2. The 100s limit: Comparing TOTAL_CONTENT_PAGE_LOAD_TIME (30s max) and TIME_TO_NON_BLANK_PAINT_MS (100s max), 30s gives a good enough limit for page load. Going from there, 50s would be a good limit for page load interactions.
3. The exponential scale becomes too coarse after a 5s+, loosing information for an important important range of page load

Steps to make this histogram more useful:

1. Change max value to 50s and drop values > 50s (don't record them)
2. Change n_buckets to 100
3. Rename probes to *_MS, as we change the buckets to avoid data collision
:wchen, any concerns with this approach to fix the histograms?
Flags: needinfo?(wchen)
Dominik, r? my proposal for fixing the histograms.
Flags: needinfo?(dstrohmeier)

I also see the need to have a more fine resolution in the low range. High values, as Harald suggests, don't have much meaning when we try to correlate first interaction to page load. I'd say that we could even go down to 30s as max value. But with the exponential scale, there is enough detail in the low values and acceptable loss of resolution for larger values.
Flags: needinfo?(dstrohmeier)
Georg, for histogram changes to avoid coming up with creative post-fixes; should we just add _V1 (and so on) to iterate on probes? I could not find any best practice in past bugs that changes probes that really made this clear and the overhead of changing probe names could add stop energy for adjusting settings for existing probes.
Flags: needinfo?(gfritzsche)
Yes, the current approach is to use versioning in the name, post-fixing with _2, _3, etc. (best to start with _2 to avoid confusion).
In the future we want to handle this a bit better, see bug 1322802.

FWIW, we have a histogram simulator tool:
Flags: needinfo?(gfritzsche)
In addition to the changes in c0, I've also done the following:

- Started the timing from first non-blank paint instead of first paint (as requested here:
- Record the time to when the input events were created instead of just before the events are dispatched to JS, that way we get a bit closer the the time the user initiated the input.
- Record the time to the first of any type of interaction (as requested here:
Assignee: nobody → wchen
Flags: needinfo?(wchen)
Attachment #8832347 - Flags: review?(amarchesini)
Comment on attachment 8832347 [details] [diff] [review]
Adjust telemetry histograms for time to first input probes.

Review of attachment 8832347 [details] [diff] [review]:

I'm not layout peer.
Attachment #8832347 - Flags: review?(amarchesini) → review?(jwatt)
As Georg recommended, can we please change the new probe names from *_MS to *_2?
Flags: needinfo?(wchen)
If you do that, please make sure that the probe description specifies that everything is measured in millisecs.
We could also do _MS_2 to make that clear.
Comment on attachment 8832347 [details] [diff] [review]
Adjust telemetry histograms for time to first input probes.

wchen: can you take account of comments 8, 9 and 10. f+ for the layout parts, but you really need a Telemetry peer review for this.
Attachment #8832347 - Flags: review?(jwatt) → feedback+
Adding telemetry peer review per comment 11

data review: This patch changes the telemetry parameters according to comment 0 to make the time to first interaction histograms more useful. It also adds a new probe to measure the time from non-blank paint to any of the currently recorded interaction types as requested in
Flags: needinfo?(wchen)
Attachment #8835029 - Flags: review?(chutten)
Attachment #8835029 - Flags: feedback?(benjamin)
Comment on attachment 8835029 [details] [diff] [review]
Adjust telemetry histograms for time to first input probes. v2

Review of attachment 8835029 [details] [diff] [review]:

Two small things

::: toolkit/components/telemetry/Histograms.json
@@ +10576,5 @@
>      "kind": "enumerated",
>      "n_values": 4,
>      "description": "Counts the number of asm.js vs WebAssembly modules instanciations, at the time modules are getting instanciated."
>    },

If TIME_TO_FIRST_CLICK_MS has never been submitted to the telemetry servers, you can just use it without the _2 suffix.

@@ +10581,2 @@
>      "alert_emails": [""],
> +    "bug_numbers": [1332511],

bug_numbers is an array, so you can include 1307675 as well as 1332511.
Attachment #8835029 - Flags: review?(chutten)
Comment on attachment 8835029 [details] [diff] [review]
Adjust telemetry histograms for time to first input probes. v2

Attachment #8835029 - Flags: feedback?(benjamin) → feedback+
Comment on attachment 8835029 [details] [diff] [review]
Adjust telemetry histograms for time to first input probes. v2

Okay, and layout r+ from me then.
Attachment #8835029 - Flags: review+
Pushed by
Adjust telemetry histograms for time to first input probes. r=jwatt, data=bsmedberg
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.