[Screenshotting] Add possibility to capture expressions #128552

dokmic · 2022-03-24T22:36:49Z

Summary

Apart from the screenshotting functionality this PR also fixes the following:

Fixes unnecessary rerendering in the Expressions Renderer React hook.
Fixes screenshotting plugin public API not to use generics.
Fixes screenshotting internal interfaces to segregate parameters instead of patching.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

…ervice

elasticmachine · 2022-03-29T10:01:33Z

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

ryankeairns · 2022-03-29T13:15:43Z

x-pack/plugins/screenshotting/public/app/app.scss

+ * 2.0.
+ */
+
+.expression {


These class names are quite generic which increases the likelihood of style overrides elsewhere in Kibana should this file ever get loaded more globally. To minimize this possibility, we recommend use a unique, 3-letter prefix such as .scrExpression and the child could be .scrExpression__viz, for example

@ryankeairns That's a good point. I followed your recommendation and prefixed that, although I didn't add the prefix to the .visualization class since we override styles of the generic class here.
Could you please take another look?

jloleysens

Great work @dokmic 🍻 , I really like the overloaded getScreenshots method.

I still need to test this and I'd like to get your thoughts on my idea about rather composing formatting and screenshotting.

jloleysens · 2022-03-29T16:00:29Z

x-pack/plugins/screenshotting/public/app/app.tsx

+import type { ExpressionRendererParams } from 'src/plugins/expressions/public';
+import { useExpressionRenderer } from '../../../../../src/plugins/expressions/public';
+import { SCREENSHOTTING_EXPRESSION, SCREENSHOTTING_EXPRESSION_INPUT } from '../../common';
+import { ScreenshotModeContext } from './screenshot_mode_context';


You can rather create a useScreenshotModeContext hook and import useContext there. One fewer thing to import here.

I would rather keep it that way since it is more up to the component implementation.

jloleysens · 2022-03-29T16:03:41Z

x-pack/plugins/screenshotting/server/formats/pdf/index.ts

 /**
 * PDFs can be a single, long page or they can be multiple pages. For example:
 *
 * => When creating a PDF intended for print multiple PNGs will be spread out across pages
 * => When creating a PDF from a Canvas workpad, each page in the workpad will be placed on a separate page
 */
-export type PdfLayoutParams = LayoutParams<typeof supportedLayouts[number]>;
+export type PdfLayoutParams = LayoutParams<Values<typeof LayoutTypes>>;


IMO it's better to be explicit about which layout types are supported. If we ever add one that shouldn't be supported in PDF this will hide it.

That is an excellent point. I've updated that. But to make it pretty, I've changed the LayoutTypes to TypeScript's enum.

jloleysens · 2022-03-29T16:05:53Z

x-pack/plugins/screenshotting/server/formats/pdf/index.ts

-    renderErrors: string[];
-  };
-}
+export interface PdfScreenshotResult {


This is a nice improvement!

x-pack/plugins/screenshotting/server/screenshots/index.ts

jloleysens

This code look good to me, great work @dokmic !

I think one missing piece is an integration or functional test to prove that the changes are working as expected but since this code is not being used anywhere I'm happy that we do this in a follow up PR.

jloleysens · 2022-03-30T11:01:28Z

src/plugins/expressions/public/react_expression_renderer/use_expression_renderer.ts

-          setEmpty(false);
-          setError(newError);
-          setLoading(false);


Was this causing unnecessary re-renders? I thought react bundled these together (or is working on bundling them together) https://reactjs.org/docs/faq-state.html#when-is-setstate-asynchronous

That's right but only for the actual rendering. It still re-renders the component (or calls the component function in our case) using that hook.

x-pack/plugins/screenshotting/server/screenshots/index.ts

tsullivan

This is really great! I am exciting to see the next steps for capturing expressions in a feature.

I would like to see an additional test in the getScreenshots method, following the code paths where it is called with an expression in the options. After that, this would LGTM

x-pack/plugins/screenshotting/server/screenshots/index.test.ts

ryankeairns

Appreciate the change, LGTM

tsullivan · 2022-04-06T22:50:30Z

x-pack/plugins/screenshotting/server/screenshots/index.test.ts

+    await screenshots
+      .getScreenshots({
+        ...options,
+        expression: 'kibana',


tsullivan

LGTM

dokmic · 2022-04-07T07:24:57Z

@elasticmachine merge upstream

kibana-ci · 2022-04-07T08:59:00Z

💛 Build succeeded, but was flaky

Test Failures

[job] [logs] Default CI Group #27 / Cases Configure Closure options change closure option successfully

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`screenshotting`	-	18	+18

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`expressions`	1701	1713	+12
`screenshotting`	12	7	-5
total			+7

Async chunks

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

id	before	after	diff
`expressions`	44.6KB	25.6KB	-19.0KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`expressions`	6	5	-1
`screenshotting`	5	4	-1
total			-2

Page load bundle

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

id	before	after	diff
`expressions`	88.3KB	98.7KB	+10.3KB
`screenshotting`	-	7.7KB	+7.7KB
total			+18.1KB

Unknown metric groups

API count

id	before	after	diff
`expressions`	2145	2158	+13
`screenshotting`	24	27	+3
total			+16

async chunk count

id	before	after	diff
`expressions`	8	5	-3

History

💔 Build #36570 failed 4988c76
💚 Build #34219 succeeded 71f01521548a8eb3a6c819ca7e33ca8fc0286aad
💔 Build #34205 failed 7b3e72e3f4d466f94b8560834dcf935c54ba115f
💔 Build #34143 failed 95fb59e4dc72fa78574b43c9e834de5ee7330ab5
💔 Build #34111 failed 2ace11392542dd262dae0c3f91d0b3744d7fd99b
💔 Build #33924 failed bd27d67111a15aea49ef338a70b128a3be11c2db

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

dokmic added 4 commits March 14, 2022 13:45

Fix the react expression renderer to avoid unnecessary rerendering

9882b99

Add screenshotting app for expressions rendering

39116c6

Refactor screenshotting to move formatting logic inside screenshots s…

ab2a73e

…ervice

Extend screenshotting plugin to support capturing expressions

18a4c65

dokmic added release_note:enhancement review (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead backport:skip This commit does not require backporting Team:AppServicesUx v8.2.0 labels Mar 24, 2022

dokmic force-pushed the feature/KS-2061 branch 5 times, most recently from 7b3e72e to 71f0152 Compare March 28, 2022 20:34

dokmic added v8.3.0 and removed v8.2.0 labels Mar 29, 2022

dokmic marked this pull request as ready for review March 29, 2022 10:01

dokmic requested review from a team as code owners March 29, 2022 10:01

dokmic requested review from tsullivan, jloleysens, vadimkibana and ppisljar March 29, 2022 10:01

ryankeairns reviewed Mar 29, 2022

View reviewed changes

botelastic bot added the Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) label Mar 29, 2022

jbudz approved these changes Mar 29, 2022

View reviewed changes

jloleysens reviewed Mar 29, 2022

View reviewed changes

jloleysens approved these changes Mar 30, 2022

View reviewed changes

Fix screenshotting shared item condition

0ea093e

tsullivan requested changes Apr 5, 2022

View reviewed changes

x-pack/plugins/screenshotting/server/screenshots/index.test.ts Show resolved Hide resolved

dokmic added 3 commits April 5, 2022 13:21

Add prefix to screenshotting CSS classes to avoid collisions

cc632f9

Add a unit test to cover screenshotting of an expression

25830f6

Make supported PDF layouts explicit

4988c76

dokmic force-pushed the feature/KS-2061 branch 2 times, most recently from c05da85 to 4988c76 Compare April 6, 2022 16:45

ryankeairns approved these changes Apr 6, 2022

View reviewed changes

tsullivan reviewed Apr 6, 2022

View reviewed changes

tsullivan approved these changes Apr 6, 2022

View reviewed changes

Merge branch 'main' into feature/KS-2061

ae4e97b

dokmic merged commit 2736f9e into elastic:main Apr 7, 2022

dokmic deleted the feature/KS-2061 branch April 7, 2022 10:01

rashmivkulkarni added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:enhancement labels Jun 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Screenshotting] Add possibility to capture expressions #128552

[Screenshotting] Add possibility to capture expressions #128552

dokmic commented Mar 24, 2022

elasticmachine commented Mar 29, 2022

ryankeairns Mar 29, 2022

dokmic Apr 6, 2022

jloleysens left a comment

jloleysens Mar 29, 2022

dokmic Apr 6, 2022

jloleysens Mar 29, 2022

dokmic Apr 6, 2022

jloleysens Mar 29, 2022

jloleysens left a comment

jloleysens Mar 30, 2022

dokmic Apr 6, 2022

tsullivan left a comment

ryankeairns left a comment

tsullivan Apr 6, 2022

tsullivan left a comment

dokmic commented Apr 7, 2022

kibana-ci commented Apr 7, 2022

API count

async chunk count

[Screenshotting] Add possibility to capture expressions #128552

[Screenshotting] Add possibility to capture expressions #128552

Conversation

dokmic commented Mar 24, 2022

Summary

Checklist

For maintainers

elasticmachine commented Mar 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jloleysens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jloleysens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsullivan left a comment

Choose a reason for hiding this comment

ryankeairns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsullivan left a comment

Choose a reason for hiding this comment

dokmic commented Apr 7, 2022

kibana-ci commented Apr 7, 2022

💛 Build succeeded, but was flaky

Test Failures

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

async chunk count

History