Skip to content

Conversation

@adamraine
Copy link
Contributor

This resolves most of the errors we were seeing for a timespan run after page load.

The solution is to mark all BE audits N/A if there were no network records in timespan mode. Conceptually, this makes some sense since ms savings are calculated from throughput in timespan mode.

Possible improvement:

Not all BE audits require network records. It was LoadSimulator, used to calculate throughput savings for all BE audits, which requires network records. Instead of always returning N/A, we could get throughput from the settings when using DT or simulated throttling, but not provided throttling. Provided throttling would need to be N/A without network records in timespan mode.

@adamraine adamraine requested a review from a team as a code owner July 30, 2021 16:03
@adamraine adamraine requested review from connorjclark and removed request for a team July 30, 2021 16:03
@google-cla google-cla bot added the cla: yes label Jul 30, 2021
@adamraine adamraine changed the title core(fr): mark n/a if no network records in timespan core(byte-efficiency): mark n/a if no network records in timespan Jul 30, 2021
@connorjclark connorjclark removed their request for review July 30, 2021 18:39
Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

nice work!


// TODO(FR-COMPAT): Reduce this number by handling the error, making N/A, or removing timespan support.
expect(erroredAudits.length).toMatchInlineSnapshot(`14`);
expect(erroredAudits.length).toMatchInlineSnapshot(`1`);
Copy link
Collaborator

@patrickhulce patrickhulce Jul 31, 2021

Choose a reason for hiding this comment

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

nice! what's the final one, #12827?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

long-tasks. It's not a byte efficiency audit but requests the page dependency graph, which throws an error when there are no network records. It requires some additional investigation before we decide to just mark as N/A.

@adamraine adamraine merged commit faf1b69 into master Aug 3, 2021
@adamraine adamraine deleted the fr-fix-timespan-errors branch August 3, 2021 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants