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

[Enterprise Search] Index Pipelines JSON Configurations #142609

Conversation

TattdCodeMonkey
Copy link
Contributor

Summary

Add the right panel with tabs for the Search Index Pipelines page, along with the JSON configurations tab.

The index pipelines get request was already returning the index specific ingest pipelines so I added the default pipeline to this result so we can get all the pipelines related to an index with a single call.

image

Index without a custom pipeline:

image

Checklist

@TattdCodeMonkey TattdCodeMonkey added release_note:skip Skip the PR/issue when compiling release notes Team:EnterpriseSearch v8.6.0 labels Oct 4, 2022
@TattdCodeMonkey TattdCodeMonkey requested a review from a team October 4, 2022 14:02
{ defaultMessage: 'View the JSON for your pipeline configurations on this index.' }
)}
footerDocLink={
<EuiLink href={docLinks.ingestPipelines} target="_blank" color="subdued">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not 100% sure if we want to use the same doc link here since it's already in the footer for the Ingest Pipelines card.

</EuiFlexItem>
</EuiFlexGroup>
<EuiSpacer size="m" />
<EuiCodeBlock language="json" overflowHeight={300} isCopyable>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this overflowHeight={300} is a bit arbitrary, is there a patter we usually follow for what to set this value to?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we have, we can try to come up with one though.

@@ -293,9 +295,15 @@ export function registerIndexRoutes({
elasticsearchErrorHandler(log, async (context, request, response) => {
const indexName = decodeURIComponent(request.params.indexName);
const { client } = (await context.core).elasticsearch;
const pipelines = await getCustomPipelines(indexName, client);
const [defaultPipeline, customPipelines] = await Promise.all([
getPipeline(DEFAULT_PIPELINE_NAME, client),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went back and forth on if I should add a new endpoint for getting a pipeline or one specific to the default pipeline. But eventually landed on returning the default ingest pipeline from this endpoint we were already calling. This works for indices that do and do not already have custom pipelines.

We already have an endpoint for the default pipeline, but it's parsing it and returning a response specific to what the ingest pipeline card/modal uses via the PipelinesLogic.

It might be worthwhile to refactor the default pipeline endpoint to return the full IngestPipeline and then do the parsing in the PipelinesLogic. Then we would not need to return the default pipeline here. but instead combine the custom pipelines and default pipeline into a single list in the frontend logic.

Copy link
Member

Choose a reason for hiding this comment

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

I like the refactor idea. Do you think we can handle this during the node architecture overhaul ?

@TattdCodeMonkey TattdCodeMonkey force-pushed the rnorris/es/pipelines-json-config-tab branch from 6db65f7 to bfd3506 Compare October 4, 2022 15:04
Copy link
Member

@efegurkan efegurkan left a comment

Choose a reason for hiding this comment

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

Looks good. I don't see a major problem. 🚀

If you can check the comments before merging that would be cool.

</EuiFlexItem>
</EuiFlexGroup>
<EuiSpacer size="m" />
<EuiCodeBlock language="json" overflowHeight={300} isCopyable>
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we have, we can try to come up with one though.

@@ -293,9 +295,15 @@ export function registerIndexRoutes({
elasticsearchErrorHandler(log, async (context, request, response) => {
const indexName = decodeURIComponent(request.params.indexName);
const { client } = (await context.core).elasticsearch;
const pipelines = await getCustomPipelines(indexName, client);
const [defaultPipeline, customPipelines] = await Promise.all([
getPipeline(DEFAULT_PIPELINE_NAME, client),
Copy link
Member

Choose a reason for hiding this comment

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

I like the refactor idea. Do you think we can handle this during the node architecture overhaul ?

Add the right panel with tabs for the Search Index  Pipelines page,
along with the JSON configurations tab. The index pipeliens get request
was already returning the index specific ingest pipelines so I added the
default pipeline to this result so we can get all the pipelines related
to an index with a single call.
The modal is closed from the pipelines logic and this createApiSuccess
listener ended up being completely unnecessary.
@TattdCodeMonkey TattdCodeMonkey force-pushed the rnorris/es/pipelines-json-config-tab branch from bfd3506 to 6389378 Compare October 4, 2022 17:21
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
enterpriseSearch 1719 1723 +4

Async chunks

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

id before after diff
enterpriseSearch 1.7MB 1.7MB +6.4KB

History

  • 💛 Build #77987 was flaky bfd3506f3c9e38bbe3d0f399b1970ac5890989cb
  • 💔 Build #77950 failed 6db65f760a55d840ddd6398e6d6793785f4f08c7

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

@TattdCodeMonkey TattdCodeMonkey merged commit c843987 into elastic:main Oct 4, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Oct 4, 2022
WafaaNasr pushed a commit to WafaaNasr/kibana that referenced this pull request Oct 11, 2022
* [Enterprise Search] pipelines json configs tab

Add the right panel with tabs for the Search Index  Pipelines page,
along with the JSON configurations tab. The index pipeliens get request
was already returning the index specific ingest pipelines so I added the
default pipeline to this result so we can get all the pipelines related
to an index with a single call.

* removed unnecessary listener

The modal is closed from the pipelines logic and this createApiSuccess
listener ended up being completely unnecessary.
WafaaNasr pushed a commit to WafaaNasr/kibana that referenced this pull request Oct 14, 2022
* [Enterprise Search] pipelines json configs tab

Add the right panel with tabs for the Search Index  Pipelines page,
along with the JSON configurations tab. The index pipeliens get request
was already returning the index specific ingest pipelines so I added the
default pipeline to this result so we can get all the pipelines related
to an index with a single call.

* removed unnecessary listener

The modal is closed from the pipelines logic and this createApiSuccess
listener ended up being completely unnecessary.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:EnterpriseSearch v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants