core(tracehouse): move files to core/lib/tracehouse/ #8956

patrickhulce · 2019-05-14T23:01:56Z

Summary
https://github.com/aslushnikov/tracium is basically a port of our files with a few things stripped out that we're encouraging external folks to use when trying to make sense of a trace. Keeping them in sync is going to be a nightmare unless we can publish from the source of truth. This is step 1 towards that goal.

(This PR) Move the required files to a lib/tracium directory
(Followup) Eliminate any dependencies from lib/tracium to the rest of LH
(Followup) Port back the patches in https://github.com/aslushnikov/tracium.
Retire https://github.com/aslushnikov/tracium / decide our monorepo strategy / do something to make external consumption and internal maintenance of tracium easier
Continue to make ongoing generic improvements as necessary

cc @aslushnikov

IMPORTANT FOR MERGE Before merging we want to keep at least two commits separate to preserve history, all the file moves are in core(tracium): move files to lib/tracium/ commit which should allow us not to lose blame history, etc

aslushnikov · 2019-05-16T09:09:05Z

Retire https://github.com/aslushnikov/tracium / decide our monorepo strategy / do something to make external consumption and internal maintenance of tracium easier

@patrickhulce Yes! If tracium lives in Lighthouse repo (which is optimal imo) and publishes to tracium's npm, than we can just deprecate and archive the aslushnikov/tracium.

Very happy this is happening! 🎉

paulirish · 2019-05-16T23:10:32Z

offline we discussed naming this tracehouse instead.

paulirish

@patrickhulce wanna rebase into your 2 separate commits for landing?

brendankenny

I'm really not a fan of having 2x of a bunch of file names in our repo, where one file is just pointing at the other, and this trend seems like it's only going to continue as we want to expose more things for external use.

I don't have a great suggestion besides real some real refactors for these files, though. Any ideas?

brendankenny · 2019-05-20T23:37:05Z

lighthouse-core/audits/bootup-time.js

@@ -7,7 +7,7 @@

 const Audit = require('./audit');
 const NetworkRequest = require('../lib/network-request');
-const {taskGroups} = require('../lib/task-groups');
+const {taskGroups} = require('../lib/tracehouse/task-groups');


brendankenny · 2019-05-20T23:37:15Z

lighthouse-core/audits/mainthread-work-breakdown.js

@@ -11,7 +11,7 @@
 'use strict';

 const Audit = require('./audit');
-const {taskGroups} = require('../lib/task-groups');
+const {taskGroups} = require('../lib/tracehouse/task-groups');


lighthouse-core/computed/trace-of-tab.js

patrickhulce · 2019-05-20T23:47:41Z

Any ideas?

Monorepo where we actually depend on our own @lighthouzzzzzzz/core packages :)

brendankenny · 2019-05-20T23:51:19Z

Monorepo where we actually depend on our own @lighthouzzzzzzz/core packages :)

the files would still be named the same without changes to either the lib/ or computed artifact files themselves, though?

patrickhulce · 2019-05-20T23:55:09Z

the files would still be named the same without changes to either the lib/ or computed artifact files themselves, though?

Oh, I see what you were saying now. Why don't we start naming our computed *-computed? this has been a problem in metrics for a long time and it's definitely annoying :)

patrickhulce · 2019-05-21T20:10:53Z

What's the final naming verdict?

Twitter poll time? 😈

paulirish · 2019-05-23T20:31:35Z

brendan voiced a few more things:

not great that we end up with a computed/trace-of-tab that's nearly empty.
we got 3 main-thread-tasks.js now.
between main-thread-tasks, tracing-processor and trace-of-tab, surely we can kill off one of those.

paulirish · 2019-05-23T20:34:40Z

What's the final naming verdict?

IMO tracehouse naming seems fine.

Why don't we start naming our computed *-computed?

agree. i'd love this. (separate PR, natch)

not great that we end up with a computed/trace-of-tab that's nearly empty.

aye. feels like a codesmell. dunno what we can do about it without adding too much magic.

we got 3 main-thread-tasks.js now.

audits could be called 'long-tasks' instead, but perhaps too late for that? the -computed fix would then disambiguate between the others..

between main-thread-tasks, tracing-processor and trace-of-tab, surely we can kill off one of those.

ayup. i think patrick was waiting to do the move first, but that seems rather tempting.

patrickhulce · 2019-05-23T20:51:54Z

not great that we end up with a computed/trace-of-tab that's nearly empty.

I'm not quite as concerned about this thing. One's the meat and the other is the adaptation of our meat to the ultra-LH-specific "computed artifact" concept which as long as we have generic library type things is relatively unavoidable. If tracehouse were it's own package on NPM we require'd in I don't think we'd be that worried about having a lightweight -computed wrapper.

audits could be called 'long-tasks' instead, but perhaps too late for that? the -computed fix would then disambiguate between the others..

The obvious step to me here is to parallel -computed and rename all our audits *-audit without changing their ids :)

Maybe I'm misunderstanding the primary concern here, but it sounds like these are all problems because the files have the same name?

i think patrick was waiting to do the move first, but that seems rather tempting.

Indeed I was, my plan for killing one of them is going to be controversial enough to warrant its own PR for discussion 😈

patrickhulce · 2019-05-30T20:53:31Z

@brendankenny what do you need to see from me to get this merged? :)

(if all looks good I will go ahead and re-rebase to have just the two commits)

brendankenny

not great that we end up with a computed/trace-of-tab that's nearly empty.

I'm not quite as concerned about this thing. One's the meat and the other is the adaptation of our meat to the ultra-LH-specific "computed artifact" concept which as long as we have generic library type things is relatively unavoidable. If tracehouse were its own package on NPM we require'd in I don't think we'd be that worried about having a lightweight -computed wrapper.

But this repo is where the files live, and if you want to edit them you just want to edit them, not step through several levels of indirection, all with the same file name, because we love our directory names.

It's a definite architecture smell.

-computed

Let's just start calling them what they really are and add -factory and tracehouse-impl (all for a single concrete implementation!) while we're at it 😢

if all looks good I will go ahead and re-rebase to have just the two commits

The lib case is huge, so let's go for it. I'm sure @paulirish checked the unit tests look good after the move, so I'll defer to his earlier LGTM on that.

patrickhulce · 2019-05-31T21:32:22Z

It's a definite architecture smell.

Ooooooooooh if we're just making commentary on the fact that it's not ideal that our repo is organized this way, then you know I wholeheartedly agree. But you already know my thoughts on this so no need for me to get the boxing gloves out 😉

aslushnikov · 2019-06-05T03:15:42Z

yay!! 🎉

What are the next steps?

patrickhulce · 2019-06-05T03:18:28Z

@aslushnikov here's the roadmap, probably 2 PRs away from matching current tracium :)

Eliminate any dependencies from lib/tracehouse to the rest of LH
Port back the patches in https://github.com/aslushnikov/tracium.
Retire https://github.com/aslushnikov/tracium / decide our monorepo strategy / do something to make external consumption and internal maintenance of tracium easier
Continue to make ongoing generic improvements as necessary

patrickhulce requested review from brendankenny and paulirish as code owners May 14, 2019 23:01

vercel bot deployed to staging May 14, 2019 23:32 View deployment

patrickhulce changed the title ~~core(tracium): move files to core/lib/tracium/~~ core(tracehouse): move files to core/lib/tracehouse/ May 20, 2019

patrickhulce force-pushed the tracium_prep branch from 2f4e089 to c8c9e72 Compare May 20, 2019 22:44

vercel bot deployed to staging May 20, 2019 22:44 View deployment

vercel bot deployed to staging May 20, 2019 23:20 View deployment

paulirish approved these changes May 20, 2019

View reviewed changes

brendankenny reviewed May 20, 2019

View reviewed changes

patrickhulce force-pushed the tracium_prep branch from 2ceb8e4 to 766e339 Compare May 20, 2019 23:53

vercel bot deployed to staging May 20, 2019 23:54 View deployment

vercel bot deployed to staging May 28, 2019 21:35 View deployment

googlebot added the cla: yes label May 28, 2019

brendankenny approved these changes May 31, 2019

View reviewed changes

patrickhulce added 2 commits May 31, 2019 16:27

core(tracehouse): move files to lib/tracehouse/

e86018e

core(tracehouse): modify files for move to tracehouse

b3cfcbd

patrickhulce force-pushed the tracium_prep branch from 85161fe to b3cfcbd Compare May 31, 2019 21:28

vercel bot deployed to staging May 31, 2019 21:28 View deployment

patrickhulce merged commit 1af2edd into master Jun 5, 2019

patrickhulce deleted the tracium_prep branch June 5, 2019 02:53

patrickhulce mentioned this pull request Jun 5, 2019

misc: update tracing-processor require #9123

Merged

This was referenced Jun 5, 2019

core(tracehouse): merge trace-of-tab and tracing-processor #9129

Merged

core(tracehouse): allow missing FCP #9174

Merged

paulirish mentioned this pull request Jul 16, 2019

Merging into Lighthouse aslushnikov/tracium#2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(tracehouse): move files to core/lib/tracehouse/ #8956

core(tracehouse): move files to core/lib/tracehouse/ #8956

patrickhulce commented May 14, 2019

aslushnikov commented May 16, 2019

paulirish commented May 16, 2019

paulirish left a comment

brendankenny left a comment

brendankenny May 20, 2019

brendankenny May 20, 2019

patrickhulce commented May 20, 2019

brendankenny commented May 20, 2019

patrickhulce commented May 20, 2019

patrickhulce commented May 21, 2019

paulirish commented May 23, 2019

paulirish commented May 23, 2019

patrickhulce commented May 23, 2019

patrickhulce commented May 30, 2019

brendankenny left a comment

patrickhulce commented May 31, 2019

aslushnikov commented Jun 5, 2019

patrickhulce commented Jun 5, 2019 •

edited

Loading

core(tracehouse): move files to core/lib/tracehouse/ #8956

core(tracehouse): move files to core/lib/tracehouse/ #8956

Conversation

patrickhulce commented May 14, 2019

aslushnikov commented May 16, 2019

paulirish commented May 16, 2019

paulirish left a comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny May 20, 2019

Choose a reason for hiding this comment

brendankenny May 20, 2019

Choose a reason for hiding this comment

patrickhulce commented May 20, 2019

brendankenny commented May 20, 2019

patrickhulce commented May 20, 2019

patrickhulce commented May 21, 2019

paulirish commented May 23, 2019

paulirish commented May 23, 2019

patrickhulce commented May 23, 2019

patrickhulce commented May 30, 2019

brendankenny left a comment

Choose a reason for hiding this comment

patrickhulce commented May 31, 2019

aslushnikov commented Jun 5, 2019

patrickhulce commented Jun 5, 2019 • edited Loading

patrickhulce commented Jun 5, 2019 •

edited

Loading