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.

Sign up for GitHub

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

Jump to bottom

[Reporting] Remove layout.selectors from PDF / PNG job parameters. #109010

Merged
merged 3 commits into from
Aug 19, 2021
Merged

[Reporting] Remove layout.selectors from PDF / PNG job parameters. #109010

merged 3 commits into from
Aug 19, 2021

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Aug 17, 2021 •
edited
Loading

Summary

Closes #107467

This PR reduces the URL length of POST URLs for PDF and PNG reports, by removing the unnecessary selectors field from job param data defined on the client-side. It's a necessary change due to issues with long POST URLs, and the fact that customizing the selectors data is not supported.

Checklist

Delete any items that are not applicable to this PR.

@tsullivan tsullivan marked this pull request as draft August 17, 2021 22:14
@tsullivan tsullivan added v7.15.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:AppServices labels Aug 17, 2021
@tsullivan tsullivan changed the title Reporting/job params selectors removal [Reporting] Remove layout.selectors from PDF / PNG job parameters. Aug 17, 2021
@tsullivan tsullivan added the bug Fixes for quality problems that affect the customer experience label Aug 17, 2021
@tsullivan tsullivan force-pushed the reporting/job-params-selectors-removal branch from df92128 to 8014631 Compare August 17, 2021 23:12
@tsullivan tsullivan marked this pull request as ready for review August 17, 2021 23:18
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServices)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-reporting-services (Team:Reporting Services)

@tsullivan
Copy link
Member Author

@elasticmachine merge upstream

@mistic mistic added v7.16.0 and removed v7.15.0 labels Aug 18, 2021
@tsullivan tsullivan added v7.15.0 and removed v7.15.0 labels Aug 18, 2021
@tsullivan
Copy link
Member Author

This is an optimization bug and it exists as of 7.13. Unfortunately it is a bit late to get this into 7.15 so it'll go out in the next release.

jloleysens
jloleysens approved these changes Aug 19, 2021
View reviewed changes
Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning this up @tsullivan ! Always great to see stuff being simplified where possible!

I did not test this locally, but I'm happy with the test coverage and to see that the CI is passing (i.e., reports are still being generated).

@jloleysens
Copy link
Contributor

@elasticmachine merge upstream

dokmic
dokmic approved these changes Aug 19, 2021
View reviewed changes
Copy link
Contributor

@dokmic dokmic left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
reporting 69.8KB 69.6KB -254.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
reporting 67.7KB 67.2KB -448.0B
Unknown metric groups

API count

id before after diff
reporting 138 135 -3

API count missing comments

id before after diff
reporting 137 134 -3

Non-exported public API item count

id before after diff
reporting 14 13 -1

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@tsullivan tsullivan merged commit 5957d31 into elastic:master Aug 19, 2021
@tsullivan tsullivan deleted the reporting/job-params-selectors-removal branch August 19, 2021 16:15
@tsullivan tsullivan mentioned this pull request Aug 19, 2021
Merged
tsullivan added a commit to tsullivan/kibana that referenced this pull request Aug 19, 2021
@tsullivan @kibanamachine
remove layout.selectors from job parameters (elastic#109010)
54e7e15 
Co-authored-by: Kibana Machine <[email protected]>
tsullivan added a commit that referenced this pull request Aug 19, 2021
@tsullivan @kibanamachine
remove layout.selectors from job parameters (#109010) (#109275)
ec96e93 
Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
@tsullivan tsullivan mentioned this pull request Oct 25, 2021
Merged
1 task
@sophiec20 sophiec20 added the (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead label Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jloleysens jloleysens jloleysens approved these changes

@dokmic dokmic dokmic approved these changes

Assignees
No one assigned
Labels
bug Fixes for quality problems that affect the customer experience (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:skip Skip the PR/issue when compiling release notes v7.16.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Reporting] Report job payload data should omit payload.layout.selectors
7 participants
@tsullivan @elasticmachine @jloleysens @kibanamachine @dokmic @sophiec20 @mistic