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 fields] Editor phase 1 #81472

Merged
merged 15 commits into from
Nov 18, 2020
Merged

[Runtime fields] Editor phase 1 #81472

merged 15 commits into from
Nov 18, 2020

Conversation

sebelga
Copy link
Contributor

@sebelga sebelga commented Oct 22, 2020

This PR adds the "runtimeFields" x-pack plugin which exposes the runtime field editor. The editor will be consumed initially by OSS apps (Index pattern management, Discover, Lens...).

[EDIT] As mentioned in the comment below, the editor won't be consumed directly by OSS apps. There will be an index_pattern_management_enhanced x-pack plugin that will register an action to edit a field. This action will be responsible of opening the runtime field editor. And it is this action that the OSS consuming apps will execute when editing or creating a runtime field.

The code in this PR has already been reviewed in several other PRs (#81766, #82116, #82464). So let's use this PR to discuss any concerns regarding the API proposed to integrate the editor in other Kibana apps.

The README.md contains detailed information on how the editor is exposed with the initial IN/OUT interfaces for consumers.

Snippet

const MyComponent = () => {
  // Access the plugin through context
  const { runtimeFields } = useAppPlugins();

  // Ref for the handler to close the editor
  const closeRuntimeFieldEditor = useRef(() => {});

  const saveRuntimeField = (field: RuntimeField) => {
    // Do something with the field
  };

  const openRuntimeFieldsEditor = async() => {
    // Lazy load the editor
    const { openEditor } = await runtimeFields.loadEditor();

    closeRuntimeFieldEditor.current = openEditor({
      onSave: saveRuntimeField,
      /* defaultValue: optionalFieldToEdit */
    });
  };

  useEffect(() => {
    return () => {
      // Make sure to remove the editor when the component unmounts
      closeRuntimeFieldEditor.current();
    };
  }, []);

  return (
    <button onClick={openRuntimeFieldsEditor}>Add field</button>
  )
}

Screenshot 2020-11-03 at 11 30 37

@sebelga sebelga added Project:RuntimeFields Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.12.0 v8.0.0 labels Oct 22, 2020
@sebelga
Copy link
Contributor Author

sebelga commented Oct 28, 2020

@elasticmachine merge upstream

@sebelga
Copy link
Contributor Author

sebelga commented Nov 3, 2020

@elasticmachine merge upstream

@sebelga sebelga marked this pull request as ready for review November 4, 2020 13:23
@sebelga sebelga requested review from a team as code owners November 4, 2020 13:23
@elasticmachine
Copy link
Contributor

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

@sebelga sebelga requested a review from jloleysens November 4, 2020 13:34
@sebelga sebelga added the release_note:skip Skip the PR/issue when compiling release notes label Nov 4, 2020
@sebelga sebelga requested review from mattkime and ppisljar November 4, 2020 15:58
@sebelga
Copy link
Contributor Author

sebelga commented Nov 4, 2020

@mattkime @ppisljar Let me know if you have any concerns in the proposed API for the integration of the editor. Cheers! 👍

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.

Happy with the proposed changes! Great work @sebelga . It would be good to see a real-world use of this component so that we can really put the API to the test, but AFAICT everything looks in order 🍻

@sebelga
Copy link
Contributor Author

sebelga commented Nov 5, 2020

Thanks for the review @jloleysens ! Yes, once integrated we will have a better sense if something is missing in the API or could be improved. I do expect it to evolve as requirements come in so we should not worry too much about it at this stage.

Comment on lines +31 to +35
export interface RuntimeField {
name: string;
type: RuntimeType;
script: string;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Using this now as a type for what the editor returns, means we've decided against a higher level "intention" abstraction, that we would e.g. save later on "the user wanted to extract the weekday" instead of the full script? Could you please elaborate a bit on why we made the decicions to always store a script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this stage, we haven't decided against anything yet 😊 What we do know for sure is that we will need to be able to return the raw script whenever the user wants to go that way and he will bypass any UI we create to help him build the script. And this is the first iteration of the editor, it only allows to write a script.
When we will improve on the UX to create the script we can decide how best to store the user intention 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

@timroes @sebelga Maybe we can split the difference. We want something that will be forward compatible but we don't necessarily want to fully define it or block progress on this pr.

I'll toss out an idea - what if a future version of RuntimeField had an editor attribute as so -

export interface RuntimeFieldEditor {
  id: string; // or something more specific
  params: { [key: string]: any; }
}

lack of an editor param would indicate the generic editor we're currently working on.

Copy link
Contributor Author

@sebelga sebelga Nov 11, 2020

Choose a reason for hiding this comment

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

This could work, but I was more seeing something like a "script definition" object. If present, then the script is generated.

interface RuntimeField {
  name: string;
  type: string;
  script: string;
  scriptDefinition?: {
    // To be defined! :)
    dateParse: {
      target: '_doc',
      field: 'eventDate',
      ... 
    },
    stringConcat: {
      ...
    },
    __meta__: {
      version: 1, // anything needed to easily upgrade between versions
    }
  }
}

But it's a bit early to define the interface IMO.

@timroes
Copy link
Contributor

timroes commented Nov 5, 2020

The way the API currently looks in the description means that every consumer needs to take care about saving those fields. What is our plan to make the integration better into the apps, so not everyone needs to put those fields into the index pattern? Will the index pattern service get a method to invoke this editor with some callbacks once it's saved?

@ppisljar
Copy link
Member

ppisljar commented Nov 5, 2020

I agree with @timroes, it seems to me the editor should integrate with IndexPattern:

  • automatically adding field to indexPattern
  • possibly automatically saving that index pattern
  • we could use the provided index pattern to autocomplete/suggest doc['fieldnames']

is there a usecase for having a RuntimeField outside of IndexPattern ?

@mattkime
Copy link
Contributor

mattkime commented Nov 5, 2020

is there a usecase for having a RuntimeField outside of IndexPattern ?

It can be defined on a field mapping.

@sebelga
Copy link
Contributor Author

sebelga commented Nov 6, 2020

What is our plan to make the integration better into the apps, so not everyone needs to put those fields into the index pattern?

I agree with @timroes, it seems to me the editor should integrate with IndexPattern

@timroes @ppisljar Thanks for the feedback. What you are suggesting is what I originally had in mind. If you search for "ExampleIntegration" in my draft Google doc, in my proposal you can see that the consuming app only provides an index pattern id (it could also be the object) and all the responsibility of fetching/updating the index pattern occurred inside the runtime field editor.

As @mattkime says, there is a use case where the user adds a runtime field to the index mappings.

@stacey-gammon told me that the responsibility of the runtime field editor ends by providing the runtime field to the consumer. We will then add a layer in between the runtimeFields plugin/editor and the OSS consuming apps (I should have mentioned that in the PR description, sorry!).

I don't have the exact API for that layer but it will be something like (correct me if I'm wrong Stacey)

const editField = (field) => {
  actionsService.getTriggerActions('EDIT_FIELD')[0].execute({ field, indexPatternObject }, callback);
};

What happens when executing the action is probably what you refer to encapsulating all the logic to update the index pattern object, right?

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

New bundle limit LGTM

}

// 3. Load the editor and open it anywhere in your app
const MyComponent = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you think about adding an example plugin so the example code would be tested and guaranteed to always be up to date? It's so tough to remember to keep code snippets like this up to date with API changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reminding me of this. Yes I will create an example plugin 👍

@mattkime mattkime self-requested a review November 16, 2020 04:49
@mattkime
Copy link
Contributor

Recently we had a discussion of how the runtime fields editor might exist inside the index pattern field editor, along side the field format editor. @sebelga mentioned that it wouldn't work with the api presented here and he's correct. At very least there's a mismatch. The runtime field editor currently takes an onSave argument, giving it a form like interface. I think we'd rather have the runtime field editor behave like a field, emitting 'onChange' events. The save button would be owned by the flyout instead of the runtime field editor.

I'm trying to think of how this might change its return value. Perhaps at some point it might return whether the runtime field value is valid but I don't think we have that functionality at this point.

@sebelga
Copy link
Contributor Author

sebelga commented Nov 16, 2020

@mattkime There are multiple ways to integrate the editor. Either calling a handler (in this case the consumer does not worry about EuiFlyout or anything) or through one of the exposed static React components. One of them is the component you mention, which corresponds to the body of the Flyout.

From what we discussed, I think the runtime field editor should be enhanced to support everything that is editable from inside index pattern management. So all-consuming app benefit from it. So on your side when clicking "edit" on a field you do a simple check:

Is it a runtime field ? yes --> open runtimeFieldEditor(field), no --> open fieldEditor(field)

The "field editor" should have a similar interface as the runtime field editor (defaultValue, onSave)

@mattkime
Copy link
Contributor

@sebelga Thanks and apologies, I did overlook the standalone component. We're in agreement.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
runtimeFields - 18 +18

Async chunks

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

id before after diff
indexManagement 1.5MB 1.5MB -24.0B

Distributable file count

id before after diff
default 42844 42848 +4

Page load bundle

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

id before after diff
indexManagement 113.8KB 114.1KB +309.0B
runtimeFields - 24.2KB +24.2KB
total +24.5KB

History

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

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

code LGTM

@sebelga
Copy link
Contributor Author

sebelga commented Nov 18, 2020

Thanks for the review @ppisljar !

I will merge this PR now. To reiterate on what we discussed offline, there will be a layer between what is exposed in this PR and the consuming OSS apps. This layer, as you suggest, will allow the following flow:

  • OSS app emits an action "openFieldEditor" with a context being something like { indexPattern, save: true }
  • that action implementation would open a side panel with the above runtime editor
  • when clicking 'save' the action implementation would add the field to the provided index pattern (and optionally save the index pattern)

cc @mattkime @timroes

@sebelga sebelga changed the title [Runtime fields] Editor [Runtime fields] Editor phase 1 Nov 18, 2020
@sebelga sebelga merged commit e3c2dcc into master Nov 18, 2020
@sebelga sebelga deleted the feature/runtime-field-editor branch November 18, 2020 08:10
sebelga added a commit that referenced this pull request Nov 18, 2020
Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: spalger <[email protected]>
# Conflicts:
#	packages/kbn-optimizer/limits.yml
phillipb added a commit to phillipb/kibana that referenced this pull request Nov 18, 2020
…o-node-details

* 'master' of github.com:elastic/kibana: (65 commits)
  update chromedriver dependency to 87 (elastic#83624)
  [TSVB] use new Search API for rollup search (elastic#83275)
  [TSVB] Y-axis has number formatting not considering all series formatters in the group (elastic#83438)
  [Logs UI] Update <LogStream /> internal state when its props change (elastic#83302)
  Add tag bulk action context menu (elastic#82816)
  [code coverage] adding plugin to flush coverage data (elastic#83447)
  [UsageCollection] Expose `KibanaRequest` to explicitly opted-in collectors (elastic#83413)
  Added eventBus to trigger and listen plotHandler event (elastic#83435)
  [Runtime fields] Editor phase 1 (elastic#81472)
  [Maps] Fix threshold alert issue resolving nested fields (elastic#83577)
  chore(NA): remove usage of unverified es snapshots (elastic#83589)
  [DOCS] Adds Elastic Contributor Program link (elastic#83561)
  Upgrade EUI to v30.2.0 (elastic#82730)
  Don't show loading screen during auto-reload (elastic#83376)
  Functional tests - fix esArchive mappings with runtime fields (elastic#83530)
  [deb/rpm] Create keystore after installation (elastic#76465)
  [rpm] Create default environment file at "/etc/sysconfig/kibana" (elastic#82144)
  [docker] removes workaround for missing crypto-policies-scripts subpackage (elastic#83455)
  [ML] Persisted URL state for the Data frame analytics jobs and models pages (elastic#83439)
  adds xpack.security.authc.selector.enabled setting (elastic#83551)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Project:RuntimeFields 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.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants