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

[Ingest pipelines] add support for registered_domain processor #99643

Merged

Conversation

sabarasaba
Copy link
Member

@sabarasaba sabarasaba commented May 10, 2021

Fixes: #97465

Release note

The Ingest Node Pipelines UI added support to configure a registered domain processor. This processor extracts the registered domain, sub-domain and top-level domain from a fully qualified domain name.

Default Description

Screenshot 2021-05-10 at 17 50 50

New Processor fields
Screenshot 2021-05-10 at 16 11 27

@sabarasaba sabarasaba added v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more Feature:Ingest Node Pipelines Ingest node pipelines management v7.14.0 labels May 10, 2021
@sabarasaba sabarasaba requested a review from a team as a code owner May 10, 2021 14:26
@sabarasaba sabarasaba self-assigned this May 10, 2021
@elasticmachine
Copy link
Contributor

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

@sabarasaba
Copy link
Member Author

@elasticmachine merge upstream

@sabarasaba sabarasaba added the release_note:feature Makes this part of the condensed release notes label May 10, 2021
Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

Nice work @sabarasaba! 🎉 Great to see support added to the UI for another processor.

Overall, everything worked as expected. I left a few minor comments. I also found one issue related to the default value for the ignore_missing parameter. I'd like to see this addressed before approving. I went through and checked our other processors using this component (out of curiosity) and found a similar bug for the csv processor. I opened #99647 to address separately.

Also, would you mind adding a screenshot of what the default description looks like to your PR? I think it might help the docs team when they review the copy.

@alisonelizabeth
Copy link
Contributor

@sabarasaba one more thing I forgot to mention 😄 - would you mind adding a release note to the PR description? You can take a look at #86163 as an example.

Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

Thanks for the quick turnaround on addressing my feedback!

I believe you'll also need to define a custom serializer, as the current behavior sets the field to undefined if it is false and strips it from the request. Perhaps we can also add a test for this behavior?

registered_domain_processor

You can check out the form lib docs for more info on how the serializer/deserializer behaves.

@sabarasaba
Copy link
Member Author

sabarasaba commented May 11, 2021

@alisonelizabeth good point! I totally missed that part in the internal implementation of the IgnoreMissingField component. I have now replaced this serializer with another one that removes it if set to true, since the api defaults the field to true we can get away with only sending it if disabled (I did it this way in order to follow the same patter that the geoIP processor uses).

Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

LGTM. Verified locally and everything works as expected now. I left a comment about the use of any, but won't block on it. Great work!

Copy link

@lockewritesdocs lockewritesdocs left a comment

Choose a reason for hiding this comment

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

Minor comments, but LGTM overall 🎉 Couldn't build/test locally because packages were missing, but updating the branch might resolve that issue.

@sabarasaba
Copy link
Member Author

@elasticmachine merge upstream

@sabarasaba
Copy link
Member Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
ingestPipelines 492 493 +1

Async chunks

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

id before after diff
ingestPipelines 680.7KB 682.0KB +1.3KB
Unknown metric groups

References to deprecated APIs

id before after diff
canvas 29 25 -4
crossClusterReplication 8 6 -2
fleet 22 20 -2
globalSearch 4 2 -2
indexManagement 12 7 -5
infra 261 149 -112
lens 67 45 -22
licensing 18 15 -3
maps 286 208 -78
ml 121 115 -6
monitoring 109 56 -53
stackAlerts 101 95 -6
total -295

History

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

cc @sabarasaba

@sabarasaba sabarasaba merged commit 391e9aa into elastic:master May 17, 2021
sabarasaba added a commit to sabarasaba/kibana that referenced this pull request May 17, 2021
…ic#99643)

The Ingest Node Pipelines UI added support to configure a registered domain processor. This processor extracts the registered domain, sub-domain and top-level domain from a fully qualified domain name.
sabarasaba added a commit that referenced this pull request May 17, 2021
… (#100210)

The Ingest Node Pipelines UI added support to configure a registered domain processor. This processor extracts the registered domain, sub-domain and top-level domain from a fully qualified domain name.
yctercero pushed a commit to yctercero/kibana that referenced this pull request May 25, 2021
…ic#99643)

The Ingest Node Pipelines UI added support to configure a registered domain processor. This processor extracts the registered domain, sub-domain and top-level domain from a fully qualified domain name.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Ingest Node Pipelines Ingest node pipelines management release_note:feature Makes this part of the condensed 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.

Add support for registered domain processor
5 participants