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: collect node stack traces #14420

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

core: collect node stack traces #14420

wants to merge 16 commits into from

Conversation

connorjclark
Copy link
Collaborator

This collects the node stack traces from the protocol, and associates them with our lhIds. Some implementation details are similar to full-page-screenshot–namely, how we must look at both of our execution contexts, and how the data is wired to the details renderer.

This PR stops short at making this information visible in the report, deferring that for later. Currently it just adds some data value to the dom on lh-node elements:

image

The artifact is stored as a mapping from lhId to the raw protocol response.

The audit is stored as a compressed representation of stack traces, de-duping frames/stacks to reduce the size. For cnn.com, this reduced the audit from being 28% of the LHR to just 2%.

@connorjclark connorjclark requested a review from a team as a code owner October 3, 2022 20:01
@connorjclark connorjclark requested review from adamraine and removed request for a team October 3, 2022 20:01
* @return {Promise<*>}
*/
async _evaluateInContext(expression, contextId) {
async _evaluateInContext(expression, contextId, returnByValue = true) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't love the changes I made here. Any suggestions?

I need to not return by value in the gatherer to get access to the backend node ids (result.objectId is not returned when returning Runtime.evaluate by value).

@connorjclark
Copy link
Collaborator Author

Hmmm... FR api tests show that CSSUsage is failing with DOM agent hasn't been enabled 🤔

@brendankenny
Copy link
Member

brendankenny commented Oct 3, 2022

This PR stops short at making this information visible in the report, deferring that for later

Can you include a sketch of this? It's not clear that all stacks of js-created-nodes should be gathered vs selectively gathering them (e.g. for some of the TraceElements, maybe), for instance.

edit: it sounds like you do want it on all elements in the report, but it would still be helpful to have a sketch of what it would look like and what specifically developers could do with that information. Would it be that helpful for most elements in the report? FPS has taken a lot of upkeep (tests, bug fixes, refactoring overhead), and ideally the tradeoff of increasing that upkeep would come from clear new functionality.

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.

None yet

4 participants