Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core(fr): split traceOfTab into timespan/navigation types #12633

Merged
merged 9 commits into from
Jun 15, 2021

Conversation

patrickhulce
Copy link
Collaborator

Summary
This PR is what it looks like to split trace of tab into two instead of going the optional properties route. Personally, I still don't think the amount of churn here is accompanied by a similar increase in clarity, but I do like finally eradicating the "trace of tab" lingering name and labeling the concepts clearly, so I'm ok with this direction if others agree that this is worth the effort :)

There is an inevitable tension between pushing timespan vs. navigation logic into audits compared to shared utilities and the split boundary (e.g. if we decide to split at TraceProcesor but preserve TraceOfTab as an object combining both processedTrace and processedNavigation as optional, we're back in the optional property boat, but individual audits mostly don't have to explicitly handle the gatherMode). I prefer using shared utilities for the handling, but I'm open to being convinced otherwise.

This PR doesn't fix most of the typedef issues in metric internals yet, nor does it include any test changes, and it's still ~500+ lines without any of the functionality in #12595 sooooo reviewers you be the judge if it's worth it 😄

Related Issues/PRs
ref #12595 (comment)
ref #11313

Copy link
Member

@adamraine adamraine left a comment

Choose a reason for hiding this comment

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

I feel positive about these changes. Even if processedTrace and processedNavigation are optional, it's much more clear what trace data I can expect from a navigation vs a timespan.

throw new Error(`${this.name} can only be computed on navigations`);
}

return Metric.compute_.call(this, data, context);
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this could be simplified by extending this class from Metric.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah I'll replace it with super.compute_ 👍

// TraceProcessor throws generic errors, but we'd like our special localized and code-specific LHError
// objects to be thrown instead.
class LHTraceProcessor extends TraceProcessor {
class ProcessedTrace {
Copy link
Member

Choose a reason for hiding this comment

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

Is this just the same as processed-trace.js for the draft so you don't have to change every import?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is. I meant to delete this file. I'd prefer not to have mismatched import names if we're already doing the massive rename split. Just rip the band-aid off at once, ya know? :)

Comment on lines 259 to 260
const processedTrace = TraceProcessor.processTrace(trace);
const {largestContentfulPaintEvt} = TraceProcessor.processNavigation(processedTrace);
Copy link
Member

Choose a reason for hiding this comment

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

Could use computed artifacts here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes! woohoo 🎉

Comment on lines +11 to +12
// TraceProcessor throws generic errors, but we'd like our special localized and code-specific LHError
// objects to be thrown instead.
Copy link
Member

Choose a reason for hiding this comment

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

I see this is a pre-existing behavior, but what is the purpose for keeping the generic errors?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The goal was to have 0 dependencies on any file outside of tracehouse so we could roll it out separately in a package for puppeteer consumption as well. That was a long time ago and the effort mostly ran out of steam without an interested consumer (see #9519 for the complete saga)

Copy link
Collaborator

Choose a reason for hiding this comment

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

drive by api bikeshed: it'd be nicer if the tracehouse lib didn't concern itself about how other consumers might want to map its errors. for future consideration in this lib or another, I think removing the static create...Error functions and instead in LH-land wrapping the resultant class in a Proxy object that try/catch'd everything and did a mapping would be cleaner. Then the lib would only need to expose Error code numbers in a static property.

@patrickhulce
Copy link
Collaborator Author

patrickhulce commented Jun 11, 2021

Shoot, lighthouse-plugin-publisher-ads uses trace-of-tab and is part of our build process.

Hey @warrengm @jburger424 heads up trace-of-tab is going to disappear. I left a compat file in the same location along with a deprecation notice on require, but we'd like to remove it.

Also the data required to compute the metrics is changing (needs GatherContext now, https://github.com/GoogleChrome/lighthouse/pull/12633/checks?check_run_id=2797959749#step:16:299)

@patrickhulce patrickhulce marked this pull request as ready for review June 11, 2021 01:28
@patrickhulce patrickhulce requested a review from a team as a code owner June 11, 2021 01:28
Comment on lines -84 to -91
if (context.settings.throttlingMethod !== 'simulate') {
const devtoolsLog = artifacts.devtoolsLogs[Audit.DEFAULT_PASS];
const metricComputationData = {trace, devtoolsLog, settings: context.settings};
const tti = Interactive.request(metricComputationData, context);
try {
minimumTimelineDuration = Math.max((await tti).timing, minimumTimelineDuration);
} catch (_) {}
}
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we need this anymore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we could keep it and just fork the behavior in screenshot-thumbnails for timespans, but honestly in practice the past few years I've felt that this tends to devolve the screenshot thumbnails into being 9 copies of the exact same final frame.

I wasn't particularly interested in the extra dance to keep this bit around and viewed it as a slight negative behavior anyway, so I just removed it instead of fixing it. Open to doing the work to support both if folks feel differently!

Copy link
Member

Choose a reason for hiding this comment

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

I don't have an attachment to it, just curious. I'm ok leaving it out.

const {trace, devtoolsLog, settings} = data;
const {trace, devtoolsLog, gatherContext, settings} = data;
if (gatherContext.gatherMode !== 'navigation') {
throw new Error(`Lantern metrics can only be computed on navigations`);
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we can get away without this check (and similar one in metric.js)?

We're doing a bunch of work to pass the gather mode in to the metrics, but is it worth it just to avoid some undefined behavior when the metrics are accidentally called during the wrong gather mode during development?

That bing said, you have don't most of the annoying work already so my opinion isn't too strong here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We certainly could (in fact to get tests to pass, I'll need to have a fallback for gatherContext for the ads plugin anyway).

My thinking here is that this direction was all about explicitness. I shared your concerns about whether the explicitness was worth it. The next PR makes processedNavigation optional in the metric implementation, and while we still have implicit workarounds like #12595 shows, it didn't feel like the spirit of explicitness to rely on those. This signal is also just critically important to integrity of the metrics, so making it a required input helps make that clear. Extra bonus: while the entire interface of trace passing is breaking anyway, it's a good time to plumb one more thing through for validation.

* @param {LH.Artifacts.ComputedContext} context
* @return {Promise<LH.Artifacts.LanternMetric>}
*/
static computeSimulatedMetric(data, context) { // eslint-disable-line no-unused-vars
Copy link
Member

Choose a reason for hiding this comment

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

Can we get rid of computeSimulatedMetric and computeObservedMetric now that we are extending Metric?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can, but I thought it was useful to still capture the narrower type of data. All of this inheritance is really in "square is not a rectangle" territory anyway which is why I opted for the wonky Metric._compute.call style.

I'm up for either.

Copy link
Member

Choose a reason for hiding this comment

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

I thought it was useful to still capture the narrower type of data

This is a good enough reason to keep it, although I wish we could enforce this type when the functions are redefined in a child class.

Leaving as is SGTM

Co-authored-by: Adam Raine <6752989+adamraine@users.noreply.github.com>
Copy link
Member

@adamraine adamraine left a comment

Choose a reason for hiding this comment

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

Overall LGTM, last few comments.

types/artifacts.d.ts Outdated Show resolved Hide resolved
types/artifacts.d.ts Outdated Show resolved Hide resolved
lighthouse-core/computed/metrics/metric.js Outdated Show resolved Hide resolved
lighthouse-core/lib/tracehouse/trace-processor.js Outdated Show resolved Hide resolved
Copy link
Member

@adamraine adamraine left a comment

Choose a reason for hiding this comment

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

LGTM nice job!

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.

5 participants