[Painless Lab] NP migration #59794

alisonelizabeth · 2020-03-10T17:07:41Z

This PR moves the painless lab plugin over to the new platform.

How to test

I'm not as familiar with this app as others might be, so please in general verify the app still behaves as it did prior to the migration.

Confirm painless scripts execute as expected
Verify "Painless Lab" appears in the Kibana plugins directory on the home page
Verify setting xpack.painless_lab.enabled: false disables the app
Verify SCSS still working as expected

Note: The license handling logic was brought over from Seb's implementation in index_management.

elasticmachine · 2020-03-10T17:07:43Z

Pinging @elastic/es-ui (Team:Elasticsearch UI)

alisonelizabeth · 2020-03-10T18:57:18Z

x-pack/plugins/painless_lab/public/plugin.tsx

+      ) as any,
+      enableRouting: false,
+      disabled: false,
+      // tooltipContent: xpackInfo.get('features.painlessLab.message'),


I'm not sure what this is for or how to test (I brought this over from the previous code). @jloleysens or @cjcenizal - any ideas?

iirc, that value would come from x-pack/legacy/plugins/painless_lab/server/lib/check_license.ts L16 as a way of informing the user that Painless Lab is not available due to a license issue.

To get parity we would need to subscribe to license$ and check the license value. We can update the UI in some other perhaps (like disabling the editors) if the ES cluster is not running min. basic license. Perhaps something to do in a follow up PR.

Ah, I see. I'm going to delete this for now.

Currently, the app is redirecting back to /dev-tools if the license is invalid. This is current functionality that I brought over. It looks like this was originally copied over from grokdebugger, as I see it is doing the same thing.

jloleysens

Great work @alisonelizabeth ! Happy to see this tech debt addressed 🙂

Left some non-blocker comments.

jloleysens · 2020-03-11T09:54:19Z

x-pack/plugins/painless_lab/public/plugin.tsx

+      ) as any,
+      enableRouting: false,
+      disabled: false,
+      // tooltipContent: xpackInfo.get('features.painlessLab.message'),


iirc, that value would come from x-pack/legacy/plugins/painless_lab/server/lib/check_license.ts L16 as a way of informing the user that Painless Lab is not available due to a license issue.

To get parity we would need to subscribe to license$ and check the license value. We can update the UI in some other perhaps (like disabling the editors) if the ES cluster is not running min. basic license. Perhaps something to do in a follow up PR.

jloleysens · 2020-03-11T10:03:37Z

x-pack/plugins/painless_lab/server/routes/api/execute.ts

+      },
+    },
+    license.guardApiRoute(async (ctx, req, res) => {
+      const body = req.body as typeof bodySchema.type;


I've also run into this 😄. The higher order function (in this case license.guardApiRoute) needs to accept the three RequestHandler generic params <P, Q, B> and pass them through to its own handler and request object.

Like here:

kibana/x-pack/plugins/watcher/server/lib/license_pre_routing_factory/license_pre_routing_factory.ts

Line 15 in babf81b

export const licensePreRoutingFactory = <P, Q, B>(

We can also review this down the line.

jloleysens · 2020-03-11T10:08:55Z

x-pack/plugins/painless_lab/common/constants.ts

+export const PLUGIN = {
+  id: 'painlessLab',
+  minimumLicenseType: basicLicense,
+  getI18nName: (i18n: any): string =>


I can't see where this is being used?

jloleysens

After running locally I noticed something I missed in my initial review. It looks like the error response is not being handled as before if invalid Painless is submitted. It now only says Bad Request.

Before it would print out a nice error message giving some more context about what exactly went wrong.

Would you mind looking into this here?

alisonelizabeth · 2020-03-11T14:03:20Z

@jloleysens I addressed your feedback and fixed the issue when a user submits an invalid painless script (great catch btw!). The UI is actually expecting a 200 in this scenario (related code). Mind taking another look?

jloleysens

@alisonelizabeth Thanks for addressing the feedback!

Left one more non-blocker comment, I'll leave that one up to you 👍

jloleysens · 2020-03-11T14:42:40Z

x-pack/plugins/painless_lab/server/routes/api/execute.ts

+        // Invalid painless script was submitted
+        // Return 200 with error object
+        if (e.body) {
+          return res.ok({
+            body: e.body,
+          });
+        }
+
+        if (isEsError(e)) {
+          return res.customError({ statusCode: e.statusCode, body: e });


I'm just wondering whether we can't collapse this section into this if check:

// Invalid painless script was submitted // Return 200 with error object if (isEsError(e)) { return res.ok({ body: e.body }); }

I'm guessing if we detect an ES error here it means that something went wrong with the call to painless script execution endpoint.

That's a valid point! Fixed.

cjcenizal

Thanks so much for doing this, Alison! I tested locally and everything looks great except for one bug I found. Let me know if you'd like help working through it.

cjcenizal · 2020-03-12T00:12:11Z

x-pack/plugins/painless_lab/server/routes/api/execute.ts

+
+const bodySchema = schema.object({
+  script: schema.object({
+    source: schema.string(),


We'll need to update this schema to support the other parameters that will be provided if the user specifies "Parameters" or the other options provided by the other "Context" choices. For example, if you specify "Parameters" as in the screenshot below the request fails because it's invalidated by this schema:

alisonelizabeth · 2020-03-12T15:40:32Z

x-pack/plugins/painless_lab/server/routes/api/execute.ts

+    schema.object({
+      params: schema.maybe(schema.any()),
+      document: schema.recordOf(schema.string(), schema.any()),
+      index: schema.string(),


I made this required, since it looks like ES requires it, but I also opened #60020 as a follow-up.

alisonelizabeth · 2020-03-12T15:47:10Z

@cjcenizal great catch! I have updated the schema, would you mind taking another look and testing again? I've confirmed adding params is working as expected now. I've tried to test changing the context, but my browser keeps freezing up (using this example). I tested on the feature branch as well and experience the same behavior, so I don't think it's related to this PR. Have you run into this before?

I also noticed a couple other issues:

The index field appears to be required by the API when changing the context, but there is no indication in the UI (opened [Painless Lab] Index field should be required in UI #60020)
Documentation links are hard-coded throughout the code (opened [Painless Lab] Documentation links are hard-coded #60024)

kibanamachine · 2020-03-12T17:27:20Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: e87cf91

History

💚 Build #32569 succeeded 54b5d15
💚 Build #32415 succeeded c76542f
💚 Build #32269 succeeded c0c0b8e
💔 Build #32200 failed d5cf2b3
💔 Build #32164 failed b681a18

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

cjcenizal

Tested locally, looks great! Thanks @alisonelizabeth. I created #60146 to track the bug you were experiencing since I can verify it also exists on the feature branch.

* Create Painless Playground app (#54578) * Replace heart script with smiley face script. (#57755) * Rename Painless Playground -> Painless Lab. (#57545) * Fix i18n namespace. * Improve smiley face proportions. - Add def keyword to Painless spec. - Temporarily fix broken highlighting. - Add small padding to main controls. * [Painless Lab] Minor Fixes (#58135) * Code restructure, improve types, add plugin id, introduced hook Moved the code execution hook to a custom hook outside of main, also chaining off promise to avoid lower level handling of sequencing. * Re-instated formatting code To improve DX the execution error response from the painless API was massaged to a more reader friendly state, only giving non-repeating information. Currently it is hard to determine the line and character information from the painless endpoint. If the user wishes to see this raw information it will be available in the API response flyout. * Remove leading new line in default script * Remove registration of feature flag * Fix types * Restore previous auto-submit request behaviour * Remove use of null and remove old comment Stick with "undefined" as the designation for something not existing. * [Painless Lab] NP migration (#59794) * Fix sample document editor. * [Painless Lab] Fix float -> integer coercion bug (#60201) * Clarify data and persistence flow. Fix floating point precision bug. * Send a string to API and ES client instead of an object. * Rename helpers lib to format. Add tests for formatRequestPayload. * Add query parameter to score context (#60414) * Fix typo and i18n * Make state init lazy Otherwise we are needlessly reading and JSON.parse'ing on every state update * Support the query parameter in requests to Painless * Fix borked i18n * Fix i18n * Another i18n issue * [Painless] Minor state update model refactor (#60532) * Fix typo and i18n * Make state init lazy Otherwise we are needlessly reading and JSON.parse'ing on every state update * Support the query parameter in requests to Painless * WiP on state refactor * Some cleanup after manual testing * Fix types and i18n * Fix i18n in context_tab * i18n * [Painless] Language Service (#60612) * Added language service * Use the correct monaco instance and add wordwise operations * Remove plugin context initializer for now * [Painless] Replace hard-coded links (#60603) * Replace hard-coded links Also remove all props from Main component * Pass the new links object to the request flyout too * Link directly to painless execute API's contexts * Remove responsive stacking from tabs with icons in them. * Resize Painless Lab bottom bar to accommodate nav drawer width (#60833) * Validate Painless Lab index field (#60841) * Make JSON format of parameters field more prominent. Set default parameters to provide an example to users. * Set default document to provide an example to users. * Simplify context's updateState interface. * Refactor store and context file organization. - Remove common directory, move constants and types files to root. - Move initialState into context file, where it's being used. * Add validation for index input. * Create context directory. * Fix bottom bar z-index. * Position flyout help link so it's bottom-aligned with the title and farther from the close button. Co-authored-by: Matthias Wilhelm <[email protected]> Co-authored-by: Jean-Louis Leysens <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Alison Goryachev <[email protected]>

* Create Painless Playground app (#54578) * Replace heart script with smiley face script. (#57755) * Rename Painless Playground -> Painless Lab. (#57545) * Fix i18n namespace. * Improve smiley face proportions. - Add def keyword to Painless spec. - Temporarily fix broken highlighting. - Add small padding to main controls. * [Painless Lab] Minor Fixes (#58135) * Code restructure, improve types, add plugin id, introduced hook Moved the code execution hook to a custom hook outside of main, also chaining off promise to avoid lower level handling of sequencing. * Re-instated formatting code To improve DX the execution error response from the painless API was massaged to a more reader friendly state, only giving non-repeating information. Currently it is hard to determine the line and character information from the painless endpoint. If the user wishes to see this raw information it will be available in the API response flyout. * Remove leading new line in default script * Remove registration of feature flag * Fix types * Restore previous auto-submit request behaviour * Remove use of null and remove old comment Stick with "undefined" as the designation for something not existing. * [Painless Lab] NP migration (#59794) * Fix sample document editor. * [Painless Lab] Fix float -> integer coercion bug (#60201) * Clarify data and persistence flow. Fix floating point precision bug. * Send a string to API and ES client instead of an object. * Rename helpers lib to format. Add tests for formatRequestPayload. * Add query parameter to score context (#60414) * Fix typo and i18n * Make state init lazy Otherwise we are needlessly reading and JSON.parse'ing on every state update * Support the query parameter in requests to Painless * Fix borked i18n * Fix i18n * Another i18n issue * [Painless] Minor state update model refactor (#60532) * Fix typo and i18n * Make state init lazy Otherwise we are needlessly reading and JSON.parse'ing on every state update * Support the query parameter in requests to Painless * WiP on state refactor * Some cleanup after manual testing * Fix types and i18n * Fix i18n in context_tab * i18n * [Painless] Language Service (#60612) * Added language service * Use the correct monaco instance and add wordwise operations * Remove plugin context initializer for now * [Painless] Replace hard-coded links (#60603) * Replace hard-coded links Also remove all props from Main component * Pass the new links object to the request flyout too * Link directly to painless execute API's contexts * Remove responsive stacking from tabs with icons in them. * Resize Painless Lab bottom bar to accommodate nav drawer width (#60833) * Validate Painless Lab index field (#60841) * Make JSON format of parameters field more prominent. Set default parameters to provide an example to users. * Set default document to provide an example to users. * Simplify context's updateState interface. * Refactor store and context file organization. - Remove common directory, move constants and types files to root. - Move initialState into context file, where it's being used. * Add validation for index input. * Create context directory. * Fix bottom bar z-index. * Position flyout help link so it's bottom-aligned with the title and farther from the close button. Co-authored-by: Matthias Wilhelm <[email protected]> Co-authored-by: Jean-Louis Leysens <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Alison Goryachev <[email protected]> Co-authored-by: Matthias Wilhelm <[email protected]> Co-authored-by: Jean-Louis Leysens <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Alison Goryachev <[email protected]>

server code NP migration

b681a18

alisonelizabeth added painless painless Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes labels Mar 10, 2020

alisonelizabeth added 2 commits March 10, 2020 14:31

migrate client code

51264c4

remove unused code

d5cf2b3

alisonelizabeth commented Mar 10, 2020

View reviewed changes

alisonelizabeth marked this pull request as ready for review March 10, 2020 19:01

alisonelizabeth requested a review from a team as a code owner March 10, 2020 19:01

alisonelizabeth requested a review from jloleysens March 10, 2020 19:02

fix TS error

c0c0b8e

jloleysens approved these changes Mar 11, 2020

View reviewed changes

jloleysens self-requested a review March 11, 2020 10:13

jloleysens requested changes Mar 11, 2020

View reviewed changes

alisonelizabeth added 2 commits March 11, 2020 09:28

address review feedback

09029f1

fix error handling for invalid painless script

c76542f

jloleysens approved these changes Mar 11, 2020

View reviewed changes

address review feedback

54b5d15

cjcenizal suggested changes Mar 12, 2020

View reviewed changes

update schema

e87cf91

alisonelizabeth commented Mar 12, 2020

View reviewed changes

cjcenizal approved these changes Mar 13, 2020

View reviewed changes

alisonelizabeth merged commit d33a82b into elastic:app/painless Mar 13, 2020

alisonelizabeth deleted the painless/np branch March 13, 2020 20:01

cjcenizal mentioned this pull request Mar 13, 2020

Create Painless Lab app #57538

Merged

13 tasks

cjcenizal added the Feature:Painless Lab Dev tool for learning Painless label Mar 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Painless Lab] NP migration #59794

[Painless Lab] NP migration #59794

alisonelizabeth commented Mar 10, 2020 •

edited

Loading

elasticmachine commented Mar 10, 2020

alisonelizabeth Mar 10, 2020 •

edited

Loading

jloleysens Mar 11, 2020

alisonelizabeth Mar 11, 2020

jloleysens left a comment

jloleysens Mar 11, 2020

jloleysens Mar 11, 2020

jloleysens Mar 11, 2020

jloleysens left a comment

alisonelizabeth commented Mar 11, 2020

jloleysens left a comment

jloleysens Mar 11, 2020

alisonelizabeth Mar 11, 2020

cjcenizal left a comment

cjcenizal Mar 12, 2020

alisonelizabeth Mar 12, 2020

alisonelizabeth commented Mar 12, 2020

kibanamachine commented Mar 12, 2020

cjcenizal left a comment

[Painless Lab] NP migration #59794

[Painless Lab] NP migration #59794

Conversation

alisonelizabeth commented Mar 10, 2020 • edited Loading

How to test

elasticmachine commented Mar 10, 2020

alisonelizabeth Mar 10, 2020 • edited Loading

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

Choose a reason for hiding this comment

jloleysens left a comment

Choose a reason for hiding this comment

alisonelizabeth commented Mar 11, 2020

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

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alisonelizabeth commented Mar 12, 2020

kibanamachine commented Mar 12, 2020

💚 Build Succeeded

History

cjcenizal left a comment

Choose a reason for hiding this comment

alisonelizabeth commented Mar 10, 2020 •

edited

Loading

alisonelizabeth Mar 10, 2020 •

edited

Loading