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

Do not make an extra call to fetch logs #94

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

brandonaaron
Copy link
Contributor

This seems to cause an issue with the chromedriver.

In regards to this comment.
After some more stumbling around and googling I tracked down this issue which hints at multiple calls to performance logs leaves out trace info. Turns out there is a call to fetch the performance logs prior to setting up the log stream. I removed this call and got the tracing info.

I ran the unit tests after this change and did not see a negative impact. I'm not sure if there is something I'm missing that requires this call.

This seems to cause an issue with the chromedriver.
@axemclion
Copy link
Owner

@brandonaaron - Thanks a lot for the PR. The extra call I was making was more to flush the logs before we start collecting them for the time range we care about. Thats why we have this.
If we do not see any tracing data, that is probably because between the time this flush happens, and the time our actions happen to get the next log flush, there were no events generated. Could you share your sample usage and the website ?

@brandonaaron
Copy link
Contributor Author

brandonaaron commented Feb 15, 2018

@axemclion I threw together a repository with two test cases and their output. I just used the same url that is used in the unit tests. One is using master and the other is using this pull request (both are modified locally to have the line uncommented to generate _perflog.json).

@brandonaaron
Copy link
Contributor Author

@axemclion Any more thoughts on this?

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