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(config): include additional trace categories #12692

Merged
merged 1 commit into from
Jul 5, 2021

Conversation

victorporof
Copy link
Contributor

Signed-off-by: Victor Porof victorporof@chromium.org

Signed-off-by: Victor Porof <victorporof@chromium.org>
@victorporof victorporof requested a review from a team as a code owner June 24, 2021 09:51
@victorporof victorporof requested review from connorjclark and removed request for a team June 24, 2021 09:51
@google-cla google-cla bot added the cla: yes label Jun 24, 2021
@victorporof victorporof changed the title Include additional trace categories core(config): include additional trace categories Jun 24, 2021
'disabled-by-default-devtools.timeline.frame',
'disabled-by-default-v8.cpu_profiler',
'disabled-by-default-v8.cpu_profiler.hires',
'latencyInfo',
Copy link
Member

@brendankenny brendankenny Jun 24, 2021

Choose a reason for hiding this comment

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

how much larger do these typically make traces? Will we need to update things like minify-trace to strip new things (or include new things) in the near future?

cc @patrickhulce

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would expect cpu_profiler.hires to be the largest of these by far on busy pages (which we were planning on enabling anyhow #8526). I think latencyInfo can be fairly chatty too if it's what I think it is.

Updating minify-trace to keep what we need from here, would be great! Could use that process to get some rough data on trace sizes for our common culprits (cnn.com, sfgate.com, theverge.com)

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

LGTM, assuming no one else has an issue here.

A run on the verge gives a trace of
current Lighthouse - 50.1MiB
this branch - 60.7MiB

So, it's a significant jump (albeit from an already very large base :). Is that going to be an issue for anyone in terms of raw storage?

We'll definitely want to follow up with minify-trace pretty soon, I think.

@victorporof
Copy link
Contributor Author

victorporof commented Jul 5, 2021

Filed #12748 for updating minify-trace.

The test failure is a known intermittent.

@brendankenny
Copy link
Member

Is that going to be an issue for anyone in terms of raw storage?

Enough time for objections has probably passed :) we can just keep an eye out for any issues

@brendankenny brendankenny merged commit 175619a into GoogleChrome:master Jul 5, 2021
@victorporof victorporof deleted the unified-categories branch July 6, 2021 08:57
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