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

[Reporting] Remove Chome stdout/stderr observables, Add Browser Logger observable #44359

Merged

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Aug 29, 2019

Summary

Previous code was not successfully capturing browser messages from the web inspector console, or for stdout/stderr of the Chromium process.

This PR fixes the log forwarding of the headless browser, wherever I found it to be possible. Any message tagged as error is tagged as an error in the Kibana logs. Any other message is tagged as debug.

This PR also completely removes the old handling of text data stream handling that were available prior to integrating Puppeteer: nssError$, fontError$, noUsableSandbox$. We can file an issue in Puppeteer to get an investigation of why we can't check for stdout messages.

The additional logging is tagged as headless-browser-console. Here are some samples:
image

Replaces: #44056

Remove handling of nssError$, fontError$, noUsableSandbox$
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-stack-services

);
logger.error(error);
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes an unrelated problem where an error object was getting concatenated with a string, making it unreadable

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@tsullivan tsullivan changed the title Add browserLogger$, processLogger$ [Reporting] Remove Chome stdin observables, Add Browser Logger observable Sep 3, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

@joelgriffith joelgriffith left a comment

Choose a reason for hiding this comment

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

LGTM


const [consoleMessage$, message$] = partition(
(msg: string) => !!msg.match(/\[\d+\/\d+.\d+:\w+:CONSOLE\(\d+\)\]/)
)(stderr$);
Copy link
Member Author

Choose a reason for hiding this comment

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

Removing this code as it does not work with the puppeteer driver. I believe this was written at a time when we launched Chromium directly, without a helper. As of now, we no longer have a way to scan the browser's stdin / stderr as a stream.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this only works if you use dumpio in puppeteer launch: https://pptr.dev/#?product=Puppeteer&version=v1.19.0&show=api-puppeteerlaunchoptions. We don't expose an option to do this, so I'm cool with removing this

Copy link
Member Author

Choose a reason for hiding this comment

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

Same here. As we have explored using dumpio before and found that it doesn't provide a stream of helpful info for us.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@tsullivan tsullivan changed the title [Reporting] Remove Chome stdin observables, Add Browser Logger observable [Reporting] Remove Chome stdout/stderr observables, Add Browser Logger observable Sep 5, 2019
@tsullivan
Copy link
Member Author

@joelgriffith and I have discussed the risk of removing the stdout/stderr observables, and we both agree that is dead code per limitations in Puppeteer. We are going to move forward with this. I will file an issue that we must try to restore the behavior that can scan text on the reason for Chromium failing to run and expose that to the user on a job-per-job basis.

@tsullivan tsullivan merged commit 24137e8 into elastic:master Sep 5, 2019
@tsullivan tsullivan deleted the reporting/fix-browser-console-logging branch September 5, 2019 20:21
tsullivan added a commit to tsullivan/kibana that referenced this pull request Sep 5, 2019
…r observable (elastic#44359)

* Add browserLogger$, processLogger$

Remove handling of nssError$, fontError$, noUsableSandbox$

* clean up messy logs being forwarded
tsullivan added a commit that referenced this pull request Sep 6, 2019
…r observable (#44359) (#44940)

* Add browserLogger$, processLogger$

Remove handling of nssError$, fontError$, noUsableSandbox$

* clean up messy logs being forwarded
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 6, 2019
…ete-for-distance_feature

* 'master' of github.com:elastic/kibana: (89 commits)
  Replace TSVB timeseries charts with elastic-charts (elastic#33558)
  [TSVB][Top N aggregation] Unable to deal with negative values (elastic#43581)
  [alerting] Adds Action Type configuration support and whitelisting (elastic#44483)
  FTR: fix WebDriver Actions calls (elastic#44605)
  [Code] add NodeRepositoriesService to watch new repositories on local node (elastic#44677)
  [skip-ci][Maps] Improve Maps intro page (elastic#44721)
  [Maps] Update titles and descriptions for data sources (elastic#44833)
  Types + Extract Integration Util (elastic#44433)
  Downgrade log level from info to debug for cases when we cannot handle authentication attempt. (elastic#44933)
  [Reporting] Remove Chome stdout/stderr observables, Add Browser Logger observable (elastic#44359)
  Update Jest script to output coverage (elastic#44447)
  [ftr] support --kibana-install-dir flag (elastic#44552)
  [WATCHER] Allow user to set a threshold value of 0 (elastic#44810)
  Remove injectI18n in dashboard plugin. (elastic#44580)
  [Graph] Save modal (elastic#44261)
  Use external script for the OIDC Implicit flow handler page. (elastic#44866)
  disable router prefixing with pluginId (elastic#44855)
  [SIEM] Fix bug on url + inspect functionality on hosts/hostDetails page (elastic#44671)
  [ML] File data viz limiting uploaded doc chunk size (elastic#44768)
  [code] Append go env variable 'GOCACHE' to go lsp spawn command. (elastic#44864)
  ...
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 6, 2019
…plate

* 'master' of github.com:elastic/kibana: (91 commits)
  [APM] Make number of x ticks responsive to the plot width (elastic#44870)
  [ML] Single metric viewer: Fix top nav refresh behaviour. (elastic#44860)
  Replace TSVB timeseries charts with elastic-charts (elastic#33558)
  [TSVB][Top N aggregation] Unable to deal with negative values (elastic#43581)
  [alerting] Adds Action Type configuration support and whitelisting (elastic#44483)
  FTR: fix WebDriver Actions calls (elastic#44605)
  [Code] add NodeRepositoriesService to watch new repositories on local node (elastic#44677)
  [skip-ci][Maps] Improve Maps intro page (elastic#44721)
  [Maps] Update titles and descriptions for data sources (elastic#44833)
  Types + Extract Integration Util (elastic#44433)
  Downgrade log level from info to debug for cases when we cannot handle authentication attempt. (elastic#44933)
  [Reporting] Remove Chome stdout/stderr observables, Add Browser Logger observable (elastic#44359)
  Update Jest script to output coverage (elastic#44447)
  [ftr] support --kibana-install-dir flag (elastic#44552)
  [WATCHER] Allow user to set a threshold value of 0 (elastic#44810)
  Remove injectI18n in dashboard plugin. (elastic#44580)
  [Graph] Save modal (elastic#44261)
  Use external script for the OIDC Implicit flow handler page. (elastic#44866)
  disable router prefixing with pluginId (elastic#44855)
  [SIEM] Fix bug on url + inspect functionality on hosts/hostDetails page (elastic#44671)
  ...
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

3 participants