core(fr): separate phase from gatherMode

[patrickhulce]

Summary
After trying to prototype out a few of the tougher gatherers we'll eventually want to convert, they required some hacky maneuvering to fit into the current phase structure of Fraggle Rock. After talking with @paulirish we came to the conclusion it'd be better if the concept of phase and gatherMode weren't so tied to together. It was convenient to understand and reason about when it all worked seamlessly, but just made things really confusing when gatherers became more complex (where do you need to write gatherer functionality to get it to run? I want this to run in timespan and navigation modes will beforeTimespan be called if I also use beforeNavigation?) .

This PR...

• Severs the name relationship between phases and gather modes by renaming the phases.
  • beforeTimespan -> startInstrumentation
  • afterTimespan -> stopInstrumentation
  • startSensitiveInstrumentation/stopSensitiveInstrumentation -> better separates the performance sensitive observations that should capture as little of Lighthouse overhead as possible (i.e. tracing, network logs, debugger, etc)
  • beforeNavigation/afterNavigation no longer needed, instrumentation methods are invoked for both timespan and navigation gatherModes and gatherers can decide if they want to alter their behavior or not.
  • snapshot -> getArtifact
• Refactors the 3 FR runners to share all of this phase collection logic so it's not repeated.
• Forces all gatherers to return their results in getArtifact. This might feel reminiscent of the ancient times when every gatherer had to store information on this before returning it in a "collect" method but it's a little different because every snapshot gatherer still won't have to touch this and timespan gatherers mostly already have to store their data on this during observations anyway, so we still get many of the benefits. This is technically an optional change, but I think it further reinforces the idea that in stop* methods the ONLY thing that should be done is to stop observing, and all heavier logic should happen in getArtifact. The fact that it's optional means if we regret this decision in the future we could always undo it later, but if we start with allowing stop* to return artifacts we can't really go back on it.
  • This also removes the ability to depend on other artifacts in stopInstrumentation phase (because the artifact is only computed in getArtifact)

Feedback I'm Looking For

• Does the above make sense?
  • Missing any phases?
  • Do we want to preserve the returning of the artifact in stopInstrumentation for ease and more dependency opportunities?
  • Anything I'm missing :)
• Any strong preferences on other phase names?

Alternative Phase Names

If you'd like to 🚲 🏠 the new phase names, here were some others thrown around...

• startObservations / endObservations
• startHeavyInstrumentation
• beginDelicateInstrumentation
• collectArtifact
• gather
• collect
• analyze

TODO

• Validate names with the team
• Update all the FR runner tests for the new model of execution
• Do final searches for any occurrences of the old names

Related Issues/PRs
ref #11313

[patrickhulce]

adriana liked "observations"

[adamraine]

Quick opinions:

• startHeavyInstrumentation > startSensitiveInstrumentation
• I like gather because it mirrors the audit function for audits.

[brendankenny]

startSensitiveInstrumentation/stopSensitiveInstrumentation

One thing we talked about is that even though this adds API surface, it actually makes gatherer usage/interdependencies easier to understand vs careful ordering in the config (plus possibly ignoring some stuff in the trace). If we come to the end of the FR transition and we find that they're overkill, only a few gatherers will be using them and it'll be easy to remove.

While writing a gatherer it won't always be obvious if things should be in sensitive instrumentation vs just regular instrumentation, while in practice almost everything should go in regular instrumentation. We should make that clear at least in docs/jsdocs ("if you're wondering if maybe your code should go in startSensitiveInstrumentation/stopSensitiveInstrumentation, it doesn't").

[brendankenny]

I've been partial to collect/collecting/collector instead of gather/gathering/gatherer for a long time, but to use those in the phase names we might have to switch from gatherer to collector :)

I was going to suggest s/collectArtifact/getArtifact (or whatever) because collectArtifact() is a little confusing for non-snapshot gatherers, since the "collecting" part isn't really happening there, it's just returning this._myArtifact, but @patrickhulce pointed out the issue with snapshot gatherers. I'm not sure of a good name that would describe both cases well.

[patrickhulce]

Outcome of meeting today:

• We don't all like any particular names, and we each have different favorites.
• getArtifact wins over collectArtifact
• *Instrumentation method names will stay as-is

[patrickhulce]

alright, bring it on reviews!