Skip to content

[Reporting][skip ci] Improve performance of Multi-URL jobs#45483

Closed
tsullivan wants to merge 10 commits intoelastic:masterfrom
tsullivan:optimize-wip-2
Closed

[Reporting][skip ci] Improve performance of Multi-URL jobs#45483
tsullivan wants to merge 10 commits intoelastic:masterfrom
tsullivan:optimize-wip-2

Conversation

@tsullivan
Copy link
Member

@tsullivan tsullivan commented Sep 12, 2019

Summary

Closes: #41299

  • Pull observables out of screenshots/index.ts
  • Make generate_pdf and generate_png handle more of the flow

There are a few pre-existing hacks with screenshot generation, so we have to make sure that fixes like this are still going to work: https://github.com/elastic/kibana/pull/31949/files

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

Copy link
Member Author

Choose a reason for hiding this comment

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

@tsullivan tsullivan changed the title --wip-- [skip ci] [Reporting] Improve performance of Multi-URL jobs Sep 13, 2019
Copy link
Member Author

Choose a reason for hiding this comment

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

If I understand correctly, this is what closes the stream on the observable, triggering the browser exit logic, and it happens in between each URL

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's how I read it as well

Copy link
Member Author

@tsullivan tsullivan Sep 13, 2019

Choose a reason for hiding this comment

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

This stuck out, because it means that there is input validation happening at execute time

Copy link
Member Author

Choose a reason for hiding this comment

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

@elasticmachine
Copy link
Contributor

💔 Build Failed

@tsullivan tsullivan changed the title [Reporting] Improve performance of Multi-URL jobs [Reporting][skip ci] Improve performance of Multi-URL jobs Sep 13, 2019
@tsullivan
Copy link
Member Author

cc @joelgriffith

Copy link
Member Author

Choose a reason for hiding this comment

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

positionElements depends on the "type" of Layout instance this is, which means the Layout class is insufficient: its instances are not polymorphic

@elasticmachine
Copy link
Contributor

💔 Build Failed

browser.waitForSelector(successSelectors, {}, logger), // finds DOM attributes for Kibana embeddables
checkForToastMessage(browser, layout, logger), // if this wins the race, there's an error on the page
]);

Copy link
Contributor

Choose a reason for hiding this comment

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

This is much more readable, and less complex, than doing the mergemap chain

Copy link
Member Author

Choose a reason for hiding this comment

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

it also seems to not work nearly as well :(

@tsullivan
Copy link
Member Author

This might not be in good enough shape to be the start of something usable. The screenshots/index.ts piece is exciting, but in testing it doesn't work as well as the previous code. I'm going to leave this open as a reference to what we could be doing in the generate_pdf and generate_png functions.

@tsullivan
Copy link
Member Author

tsullivan commented Sep 30, 2019

I'm going to close this Draft PR, because I think the original idea is going to be a less than ideal solution.

The problem we're facing is that multi-URL PDFs are generated by creating a new browser driver observable for each URL. Instead of creating the browser driver first and calling screenshotObservable in a loop, it feels like a better change would be just to make the screenshot observable function take an array of strings as URLs.

The reason for this thinking is seeing how the current code makes it easy for outside consumers to import the screenshot observable and start making their own screenshots. In a change like the one from this draft PR, the outside consumer would also have to grab the various libraries needed to instantiate the browser driver, and then pass it to the screenshots observable.

@tsullivan tsullivan closed this Sep 30, 2019
@tsullivan tsullivan deleted the optimize-wip-2 branch September 30, 2019 16:32
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.

[Reporting] Lots of room for improvement when there are multiple relativeUrls in the POST URL

3 participants