feat: introduce step level tracing #369
Conversation
vigneshshanmugam
commented
Aug 27, 2021
•
vigneshshanmugam
commented
- fix Introduce step level tracing for perf metrics #358
- Updates the lighthouse processor with new changes that accounts for different modes - navigation, snapshot etc
- Trace computation has been updated to handle the navigation changes inside steps
- Plugin manager is cleaned up to add support for step level and journey level plugins with new API such as register, unregister etc.
- Each Plugin follows the same interface to keep things simpler in the PluginManager.
- new Plugin(driver)
- plugin.start
- plugin.stop
- JSON reporter now writes Metrics and Traces for each step if there is a navigation event of interest.
vigneshshanmugam
force-pushed
the
step-tracing
branch
from
cf40f59
to
c9fad76
Compare
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪 |
| @@ -625,4 +625,35 @@ describe('runner', () => { | |||
| const blobs = screenshotDocs.map(data => data.blob); | |||
| expect(blobs[0]).not.toEqual(blobs[1]); | |||
| }); | |||
|
|
|||
| it('run - capture trace step level', async () => { | |||
There was a problem hiding this comment.
@andrewvc @dominiqueclarke This test provides the complete picture.
| this.writeMetrics(journey, step, 'experience', metrics); | ||
| if (filmstrips) { | ||
| // Write each filmstrip separately so that we don't get documents that are too large | ||
| filmstrips.forEach((strip, index) => { |
There was a problem hiding this comment.
Does this affect timing? I thought we saved it for the end on purpose so as not to affect journey timing as much
There was a problem hiding this comment.
We left it to the end to make sure to run the tracer only once per journey, but we are already running trace for each step anyway so there is no harm in also capturing filmstrips.
| break; | ||
| case 'browserconsole': | ||
| instance = new BrowserConsole(this.driver.page); | ||
| instance.start(); | ||
| instance = new BrowserConsole(this.driver); |
There was a problem hiding this comment.
Any reason you changed the constructor parameters aside from consistency? Just wondering.
There was a problem hiding this comment.
Not really, only to be consistent across all plugins. Makes things easier.
| @@ -41,7 +41,7 @@ const supportedMetrics = new Set<string>([ | |||
| 'JSHeapTotalSize', | |||
| ]); | |||
|
|
|||
| export interface Metrics { | |||
| export interface PageMetrics { | |||
There was a problem hiding this comment.
What exactly is meant by Page in this context? Is it metrics for the playwright Page class, and thus metrics for that particular browser tab on a per step level?
There was a problem hiding this comment.
Its per browser tab which alias to Page in playwright context.
| } | ||
|
|
||
| onStep(step: Step) { | ||
| this.get(BrowserConsole) && (this.get(BrowserConsole)._currentStep = step); | ||
| this.get(NetworkManager) && (this.get(NetworkManager)._currentStep = step); | ||
| (this.get('browserconsole') as BrowserConsole)._currentStep = step; |
There was a problem hiding this comment.
Could create a setStep function that takes the plugin type and the step for reusability.
There was a problem hiding this comment.
Good point, But we don't want to call this for each plugin instead abstract away from runner and handle it in single place in PluginManger which handles the logic of assigning to respective plugins.
