Skip to content

Ingest pipeline best practices #1381

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 37 commits into from
Aug 20, 2025
Merged

Ingest pipeline best practices #1381

merged 37 commits into from
Aug 20, 2025

Conversation

philippkahr
Copy link
Contributor

@philippkahr philippkahr commented May 7, 2025 •
edited
Loading

based on the discussions here: #1052

this is my first PR against the docs, and I am building a couple of new pages. I think it makes sense to split it out. I am putting it into that part of the docs. https://www.elastic.co/docs/manage-data/ingest/transform-enrich/ingest-pipelines The tips and tricks are generic and not specific to just o11y, or security.

image

@philippkahr
Copy link
Contributor Author

philippkahr commented May 7, 2025 •
edited
Loading

There are a couple of things I need help with.

  • Can someone proof read it and give suggestion on the ease of understanding.
  • Does the file order make sense in the way I put it? Should we do an additional subfolder?
  • Can you read through it please and here and there I think we can add links to different docs, like when I say remove. processor, we should link to the remove processor probably?
  • Not a 100% convincend that common mistakes is the correct heading

@philippkahr philippkahr added Team:Platform Issues owned by the Platform Docs Team documentation Improvements or additions to documentation enhancement New feature or request labels May 7, 2025
@kilfoyle kilfoyle added Team:Obs Issues owned by the Observability Docs Team and removed Team:Platform Issues owned by the Platform Docs Team labels May 7, 2025
@kilfoyle
Copy link
Contributor

kilfoyle commented May 7, 2025

Thanks a lot for opening this Philipp! I've added the "Team:Obs" label since under the new docs organization that's where the ingest content will land.

@philippkahr philippkahr requested review from a team as code owners May 22, 2025 08:29
@colleenmcginnis colleenmcginnis added the Team:Ingest Issues owned by the Ingest Docs Team label May 27, 2025
@alexandra5000 alexandra5000 self-assigned this May 28, 2025
colleenmcginnis
colleenmcginnis reviewed May 30, 2025
View reviewed changes
Copy link
Contributor

@colleenmcginnis colleenmcginnis left a comment

Choose a reason for hiding this comment

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

@philippkahr I started reviewing this PR, but I didn't get very far (yet!). There's a lot of content to get into! I'm just going to post the comments/questions/suggestions I have so far so I can see if I'm on the right track. I can jump back in next week.

Some themes in my early feedback include:

  • I see some opportunities to simplify the examples to really emphasize the point you're making in each section.
  • It might be helpful to write out in plain language what the example is trying to achieve before jumping into a code snippet. (I provided a couple suggestions below.)
  • There are probably opportunities to remove redundant information.

manage-data/ingest/transform-enrich/common-mistakes.md Outdated
Comment on lines 56 to 79
### Contains operation and null check

This includes an initial null check, which is not necessary.

```painless
"if": "ctx.event?.action !=null
&& ['bandwidth','spoofed syn flood prevention','dns authentication','tls attack prevention',
'tcp syn flood detection','tcp connection limiting','http rate limiting',
'block malformed dns traffic','tcp connection reset','udp flood detection',
'dns rate limiting','malformed http filtering','icmp flood detection',
'dns nxdomain rate limiting','invalid packets'].contains(ctx.event.action)"
```

This behaves nearly the same:

```painless
"if": "['bandwidth','spoofed syn flood prevention','dns authentication','tls attack prevention',
'tcp syn flood detection','tcp connection limiting','http rate limiting',
'block malformed dns traffic','tcp connection reset','udp flood detection',
'dns rate limiting','malformed http filtering','icmp flood detection',
'dns nxdomain rate limiting','invalid packets'].contains(ctx.event?.action)"
```

The difference is in the execution itself which should not matter since it is Java under the hood and pretty fast as this. In reality what happens is the following when doing the first one with the initial: `ctx.event?.action != null` If action is null, then it will exit here and not even perform the contains operation. In our second example we basically run the contains operation x times, for every item in the array and have `valueOfarray.contains('null')` then.
Copy link
Contributor

Choose a reason for hiding this comment

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

This example confuses me. Why would you want to run the contains operation n times if you already know ctx.event.action is null and it's going to return false.

@alexandra5000 alexandra5000 removed their assignment Jun 6, 2025
@philippkahr philippkahr mentioned this pull request Jun 10, 2025
Merged
colleenmcginnis
colleenmcginnis reviewed Jun 24, 2025
View reviewed changes
Copy link
Contributor

@colleenmcginnis colleenmcginnis left a comment

Choose a reason for hiding this comment

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

Some comments on the remaining pages, manage-data/ingest/transform-enrich/error-handling.md and manage-data/ingest/transform-enrich/general-tips.md.

@colleenmcginnis colleenmcginnis self-requested a review July 17, 2025 02:26
@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 17, 2025 •
edited
Loading

🔍 Preview links for changed docs

@colleenmcginnis
Copy link
Contributor

I left some suggestions and a proposal in philippkahr#2. Let me know what you think. 🙂

colleenmcginnis
colleenmcginnis reviewed Aug 11, 2025
View reviewed changes
Copy link
Contributor

@colleenmcginnis colleenmcginnis left a comment

Choose a reason for hiding this comment

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

Some final suggestions on the new content added in 7f5de7c.

@philippkahr philippkahr enabled auto-merge (squash) August 13, 2025 06:59
@colleenmcginnis
Copy link
Contributor

👋 @elastic/admin-docs can we get a code owner review on this PR?

shainaraskas
shainaraskas approved these changes Aug 18, 2025
View reviewed changes
Copy link
Collaborator

@shainaraskas shainaraskas left a comment

Choose a reason for hiding this comment

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

not sure how we ended up codeowners for this. approving to unblock :)

@philippkahr philippkahr merged commit 67636a7 into elastic:main Aug 20, 2025
7 of 8 checks passed
@philippkahr philippkahr deleted the best-practices-ingest-pipelines branch August 20, 2025 13:46
philippkahr added a commit that referenced this pull request Aug 26, 2025
@philippkahr @theletterf
ingest pipeline docs "ingest lag" (#1672)
1a4d206 
#1381 relates to this, we
pulled this file out as it's standalone for better review

---------

Co-authored-by: Fabrizio Ferri-Benedetti <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stefnestor stefnestor stefnestor left review comments

@colleenmcginnis colleenmcginnis colleenmcginnis approved these changes

@shainaraskas shainaraskas shainaraskas approved these changes

Assignees

No one assigned

Labels

documentation Improvements or additions to documentation enhancement New feature or request Team:Ingest Issues owned by the Ingest Docs Team Team:Obs Issues owned by the Observability Docs Team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@philippkahr @kilfoyle @colleenmcginnis @eedugon @stefnestor @shainaraskas @alexandra5000