Skip to content

Conversation

@scottjehl
Copy link
Contributor

Not yet ready to merge!

First pass on messaging for a checkbox that we'll offer when a result has slow ttfb and seems to be skewed for bots.

Live example of this message showing in a slow result:
image
image

… has slow ttfb and seems to be skewed for bots.
@scottjehl
Copy link
Contributor Author

Note: there's a todo in there noting that we (seemingly at least) need a public function to grab the median ttfb or other metric. There are similar functions to grab the median run number but not the data, from this particular file's access at least. I am still looking though! :)

@tkadlec
Copy link
Contributor

tkadlec commented Sep 20, 2021

@scottjehl What do you think about only showing this message if all the test runs have a TTFB over that threshold to avoid a situation where one load really is that much slower?

If we did that, we'd need to loop through each run to get the TTFB for each OR write a function that can does that when provided with a particular metric.

@scottjehl
Copy link
Contributor Author

Oh, yeah that's even better. Good idea, thanks. I'll aim to make a function that returns an array from the runs of a particular passed metric.

@scottjehl
Copy link
Contributor Author

scottjehl commented Sep 20, 2021

@tkadlec Instead of a new function I was able to use one that could be made public. It returns the values from a given metric from all runs, so I just got the min of those. Can you give that a quick glance? Updated design:

image
image

@scottjehl scottjehl requested a review from tkadlec September 20, 2021 21:17
@scottjehl scottjehl self-assigned this Sep 20, 2021
Copy link
Contributor

@tkadlec tkadlec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code comments? What is this new sorcery? ;)

Copy link
Contributor

@tkadlec tkadlec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great!

@tkadlec tkadlec merged commit 35f1eea into master Sep 20, 2021
@scottjehl scottjehl deleted the 1536 branch September 20, 2021 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants