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(tracing-processor): throw on no top level events #5878

Merged
merged 2 commits into from
Aug 21, 2018

Conversation

patrickhulce
Copy link
Collaborator

We were throwing on no toplevel events but only in the observed case, this PR adds the check in more places so we should fail all metrics all things that rely on the page dependency graph if there are no toplevel events.

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! Nice fix.

Should we also add something to GatherRunner? If a trace was collected, assertHasToplevelEvents (similar to checking for a main resource)? Or is that overkill

@@ -17,6 +17,20 @@ const SCHEDULABLE_TASK_TITLE_ALT2 = 'ThreadControllerImpl::RunTask';
const LHError = require('../errors');

class TraceProcessor {
/**
* There should *always* be at least one top level event, having 0 typically means something is
* drastically wrong with the trace and would should just give up early and loudly.
Copy link
Member

Choose a reason for hiding this comment

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

would should
:)

@patrickhulce
Copy link
Collaborator Author

If a trace was collected, assertHasToplevelEvents (similar to checking for a main resource)? Or is that overkill

Seems a bit like overkill to me, this is already the entire performance section basically, so I think we're pretty covered.

@patrickhulce patrickhulce changed the title core(tracing-processor): throw on no toplevel events core(tracing-processor): throw on no top level events Aug 21, 2018
@patrickhulce patrickhulce changed the title core(tracing-processor): throw on no top level events core(tracing-processor): throw on no toplevel events Aug 21, 2018
@patrickhulce patrickhulce changed the title core(tracing-processor): throw on no toplevel events core(tracing-processor): throw on no top level events Aug 21, 2018
@brendankenny
Copy link
Member

prlin is fixd :)

@patrickhulce patrickhulce merged commit bac76ec into master Aug 21, 2018
@patrickhulce patrickhulce deleted the no_toplevel_fail_loudly branch August 21, 2018 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants