[Ingest pipelines] Upload indexed document to test a pipeline

[alisonelizabeth]

This PR adds the ability to upload a document(s) from an existing index when testing a pipeline.

Fixes #74796

Skipping release note as I think this change can fall under the general improvements made to the debugging workflow. Release note added on #74964.

How to review

Testing instructions

1. Create a pipeline (for a sample pipeline, see: [Ingest pipelines] Test pipeline enhancements #74964).
2. Create an index with at least one document, or use the kibana sample data. For example:

PUT /test/_doc/1
{
  "name": "John Doe"
}

3. Click the "Add documents" link
4. Add a sample document in the "Documents" code editor (you can just use the example in the help text)
5. Click the "Import existing documents" accordion
6. Add the index name and document ID of the document from step 2 and click "Import". Note: You can use the link to "Discover" to help find the document to use.
7. Verify the document was added successfully, the document added in step 4 still exists, and you are able to run the pipeline.

UX

I'm interested in any design feedback on this. Two questions in particular on my end:

• Does it make sense to have a single form to import documents (particularly in the case where you might want to add more than one document)? I first considered doing something similar to what we see in the mappings editor for the relationships parameter, where you could add multiple rows, but I'm not sure how useful this is.

Example from mappings editor:

• Thoughts on having the imported document populate the code editor below? I think we had initially discussed potentially having some sort of preview of the document you selected. However, I thought adding it to the code editor might be nice in the scenario where you might want to tweak the imported document, or also add a second sample document with JSON.

Screenshots

Initial view:

Accordion open:

Field errors:

API error:

[elasticmachine]

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

[cjcenizal]

🔥 This is awesome! I tested locally and the UX and code both look solid to me. I had some suggestions for enhancing the UX and making the code a bit more robust around some corner cases, but nothing worth blocking on.

Does it make sense to have a single form to import documents (particularly in the case where you might want to add more than one document)?

I found this UX to be fairly intuitive. I think it's useful and works well. I think adding a list of multiple indices and documents would add unnecessary complexity.

Thoughts on having the imported document populate the code editor below?

This was a really cool interaction! It was straightforward to load up documents and then find and review them in the code editor. I think we can provide more feedback when a document is successfully loaded, per one of my comments below.

[cjcenizal]

It seems possible for the component to be mounted at the time getDiscoverUrl is called, but unmounted by time time discoverUrl resolves. In that case, don't we want to move the umounted check a little later in the control flow?

const getDiscoverUrl = async (): Promise<void> => {
  const discoverUrlGenerator = services.urlGenerators.getUrlGenerator(
    'DISCOVER_APP_URL_GENERATOR'
  );
  const discoverUrl = await discoverUrlGenerator.createUrl({ indexPatternId: undefined });
  if (!unmounted) {
    setDiscoverLink(discoverUrl);
  }
};

[cjcenizal]

I'm not very familiar with how our URL generators work, so maybe this is a dumb question. What happens if Discover is disabled? Will discoverUrlGenerator or maybe discoverUrl resolve to undefined? In which case should we check that it's defined before consuming them?

[alisonelizabeth]

Thanks for bringing this up!

I read through the docs again, and there is a isDeprecated flag that is returned from the url generator. I added a check for this in the code.

[cjcenizal]

TIL about the isDeprecated flag! I tested this out locally by setting discover.enabled: false and I ended up getting a console error:

I read the docs a bit more closely and I think isDeprecated is intended to address the situation when an app has changed the way it stores state in its URLs, like the way Discover stores filter, query, and sort state in the URL. I'm guessing the above error is the intended way they surface a generator missing due to a plugin being disabled. In this case, I suggest use a try/catch to detect the scenario and handle it gracefully. Here's an example of what I mean, though ideally we'd inspect the type of error and bubble it up if it's not a no generator found error:

const getDiscoverUrl = async (): Promise<void> => {
  let isDeprecated;
  let createUrl;

  try {
    ({ isDeprecated, createUrl } = services.urlGenerators.getUrlGenerator(
      'DISCOVER_APP_URL_GENERATOR'
    ));
  } catch(e) {
    // Discover is not enabled.
    setDiscoverLink(undefined);
    return;
  }

  if (isDeprecated) {
    setDiscoverLink(undefined);
    return;
  }

  const discoverUrl = await createUrl({ indexPatternId: undefined });

  if (isMounted.current) {
    setDiscoverLink(discoverUrl);
  }
};

[cjcenizal]

I created #78344 to improve the URL generator docs or API for this scenario.

[alisonelizabeth]

Ah, I see. Thanks for looking into this more closely! I've updated the code and added a comment with a link to the issue you created.

[cjcenizal]

Just wondering why table and column are used in the IDs here and on line 63?

[alisonelizabeth]

Oops! I started with a slightly different design, and forgot to update the the IDs here.

[cjcenizal]

Nit: should be idRequiredErrorMessage based on the guidelines.

[cjcenizal]

Nit: should be indexRequiredErrorMessage based on the guidelines.

[cjcenizal]

If you're adding multiple documents from the same index, it can be useful to retain the form state so you don't need to keep re-entering the index. Similarly, if the IDs happen to be sequential I can imagine it being useful to retain the document ID so you can just increment them easily. At the very least it's helpful to see which document you just added so you can refer to it when choosing the next one. For these reasons I suggest retaining the form state and removing this reset.

I think it would be helpful feedback to surface a success callout just above the form when the request is successful, like this:

[alisonelizabeth]

These are great points. I removed the reset and added the success callout.

[cjcenizal]

If the user clears out the content of the "Documents" editor so that it's entirely blank and then tries to import a document, they get this error:

We can fix this by changing this line to:

const { documents: existingDocuments = [] } = formatData();

[alisonelizabeth]

Great catch!

[cjcenizal]

If the network's a bit slow and the user navigates away before this request resolves, then we'll get an error about updating state on an unmounted component:

It's a bit verbose, but here's one way we can solve this problem. We can track unmounted state like this:

  const isMounted = useRef(false);

  useEffect(() => {
    isMounted.current = true;

    return () => {
      isMounted.current = false;
    };
  }, []);

Then we can check this state before updating any state after the request resolves:

if (isMounted.current) {
  setIsLoadingDocument(false);

  if (error) {
    setLoadingDocumentError(error);
    return;
  }

  onAddDocuments(document);
  form.reset();
}

To reduce the verbosity we could extract the logic for tracking isMounted state into its own hook per the last few comments on this discussion: https://gist.github.com/jaydenseric/a67cfb1b809b1b789daa17dfe6f83daa

[alisonelizabeth]

Great catch! I've added a use_is_mounted hook.

[cjcenizal]

I found this phrasing a little confusing... "existing" had me trying to draw a comparison to a "non-existing" document, and "import" made me think of uploading a file. How about we use the verb "Add" instead, and instead of "existing" we clarify that the document comes from an index? I think this summarizes the help text of this section nicely.

[cjcenizal]

WDYT of surfacing the number of documents somewhere? For example in the label:

[alisonelizabeth]

I like this idea. It would require some refactoring though, since the field label is currently defined in the form schema. I'm going to leave as-is for now.

[alisonelizabeth]

Thanks for the very helpful review @cjcenizal! I addressed your feedback via 4b4c81b.

[mdefazio]

Sorry I'm just now getting to this. Here's some ideas for the layout:

• Using panels in flyouts adds a lot of extra UI noise and I think because it's an accordion we can just use euiColorLightestShade as the background color for the content inside it.
• Because the index and document can be quite long, I think it's worthwhile to have these inputs on their own line. Yes it adds a lot of height to the section, but the user opened this up and can easily close it when done.
• I moved the success message to be beside the button. If I continuously add documents, this can help avoid any jumping in the UI.
• With this in mind, I added a 'Clear documents' text button at the top right of the documents window. Does what it says in case I want to swap out documents.
• (Tiny nit, would be nice to hit enter from the document id input to add the document)
• Does it make sense to use a search dropdown for the index selector like we have in Discover?

Here's a mockup illustrating these edits:

[alisonelizabeth]

@mdefazio thanks for the great suggestions! I've implemented most of them. Would you mind taking another look when you get a chance?

With this in mind, I added a 'Clear documents' text button at the top right of the documents window. Does what it says in case I want to swap out documents.

I like this idea. I think this work more closely aligns with #78183 though, so I will add this functionality as part of that PR.

Does it make sense to use a search dropdown for the index selector like we have in Discover?

I think this would be a nice enhancement. I'm going to put this on hold for now, but will consider adding this capability in a separate PR.

Screenshot with latest changes:

[mdefazio]

LGTM!

[alisonelizabeth]

@elasticmachine merge upstream

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 7ca5d5c

History

• 💔 Build #77021 failed a1aa8d8
• 💔 Build #76612 failed 4b4c81b
• 💚 Build #75981 succeeded d1b6fd2
• 💔 Build #75926 failed a32e5c1
• 💔 Build #75869 failed ef10904

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