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

[Runtime field editor] Add server side for preview functionality #100029

Conversation

sebelga
Copy link
Contributor

@sebelga sebelga commented May 13, 2021

This PR contains the core logic of the preview functionality of runtime fields inside the index pattern field editor.
It does not contain any UI yet so I output for now the result in the browser console.

How to test

  • Add Kibana sample data logs
  • Navigate to the index pattern management and select the sample data logs index pattern
  • Click on "Add field" and open the Chrome dev tools to see the console
  • Give a name to the field (e.g "foo")
  • Enable the "Set value" toggle and add a script (e.g. emit("bar"))
  • You should see the field preview in the console
Field preview: {
    "key": "foo",
    "value": "bar"
}
  • Now update the script with the following where we read data from the index pattern document
Long bytesToGB = doc['machine.ram'].value / 1073741824;

emit(bytesToGB + 'GB');
  • You should see the updated preview
Field preview: {
    "key": "foo",
    "value": "8GB"
}

Notes on implementations

Added <FieldEditorProvider />

To avoid property drilling I've added a fieldEditorContext to pass down services and context value to components down the tree. In this PR I haven't modified the existing code to reduce the amount of changes. In a following PR I will refactor to code to make use of the new context.

New top level __jest__ folder

In order to follow the pattern put in place in all of our apps I have created a top level __jest__ folder to add the component integration tests. Now that we have server side endpoint to test, the helpers will help mock the HTTP request response easily and test different scenarios in the UI.
In a following PR I will refactor the current component integration tests and move them to this folder.

Screenshot 2021-05-17 at 12 27 28

@sebelga sebelga added Feature:Data Views Data Views code and UI - index patterns before 8.0 Team:AppServices Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.14.0 v8.0.0 labels May 13, 2021
@sebelga sebelga force-pushed the runtime-field-editor/preview-endpoint-2 branch from d85f68d to 4d680bc Compare May 14, 2021 09:35
@sebelga sebelga force-pushed the runtime-field-editor/preview-endpoint-2 branch from deecf30 to f394149 Compare May 17, 2021 09:42
@sebelga sebelga marked this pull request as ready for review May 17, 2021 14:04
@sebelga sebelga requested a review from a team as a code owner May 17, 2021 14:04
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@sebelga sebelga requested a review from mattkime May 17, 2021 14:04
@sebelga sebelga requested a review from a team as a code owner May 18, 2021 14:52
@sebelga
Copy link
Contributor Author

sebelga commented May 18, 2021

@elastic/kibana-operations I had to update the plugin size limit as we have a tech debt on the field editor and need to change how we expose the field format editors to other plugins so make it lazy loaded instead of eagerly @mattkime . Do we know how many plugins currently use the public interface of the index pattern field editor to access the field format editors?

Screenshot 2021-05-18 at 15 48 41

@sebelga
Copy link
Contributor Author

sebelga commented May 19, 2021

@elasticmachine merge upstream

@sebelga
Copy link
Contributor Author

sebelga commented May 19, 2021

@elasticmachine merge upstream

@mattkime
Copy link
Contributor

@sebelga We should definitely lazy load the field format editor. I'm not aware of any use cases where it isn't used as part of the index pattern field editor but I might be misunderstanding your question.

#100366

@sebelga
Copy link
Contributor Author

sebelga commented May 20, 2021

@mattkime You've exposed the fieldFormatEditors publicly for other plugins (https://github.com/elastic/kibana/blob/master/src/plugins/index_pattern_field_editor/public/plugin.ts#L41) So I wondered which plugin currently make use of it to know the impact of exposing an async loader instead of the format editors directly. And if no plugin use the format editors maybe we could remove it from the public interface?

@sebelga
Copy link
Contributor Author

sebelga commented May 20, 2021

@elasticmachine merge upstream

@mattkime
Copy link
Contributor

So I wondered which plugin currently make use of it to know the impact of exposing an async loader instead of the format editors directly. And if no plugin use the format editors maybe we could remove it from the public interface?

I haven't taken a look through the code but I know that its important to expose it for third party plugins. Custom field formatters are one of the most common reasons to create a third party plugin.

@sebelga
Copy link
Contributor Author

sebelga commented May 25, 2021

@elasticmachine merge upstream

});
};

return handleEsError({ error, response: res, handleCustomError });
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think in our other plugins we've tended to inject the handleEsError dependency, so line 31 would be something like this:

export const registerFieldPreviewRoute = ({ router, lib: { handleEsError } }: RouteDependencies): void => {

Not a blocker but it will make the code a little more consistent and would make it easier to mock this dependency if we ever write unit tests for these route handlers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I remember that's the pattern I introduced in our plugins. I am not planning in adding unit tests for API route handlers as I tend to prefer integration tests for them.
I might update it though in future PR for the sake of alignement. 👍

@mattkime
Copy link
Contributor

We should probably look at breaking out field format editors into its own plugin.

Copy link
Contributor

@mattkime mattkime left a comment

Choose a reason for hiding this comment

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

code runs well, looks good!

@sebelga
Copy link
Contributor Author

sebelga commented May 26, 2021

Thanks for the review @mattkime and @cjcenizal !

@sebelga sebelga merged commit ec64428 into elastic:runtime-field-editor/preview-field-workstream May 26, 2021
@sebelga sebelga deleted the runtime-field-editor/preview-endpoint-2 branch May 26, 2021 09:09
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@cjcenizal cjcenizal added the release_note:skip Skip the PR/issue when compiling release notes label Aug 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Data Views Data Views code and UI - index patterns before 8.0 release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants