Suggested copy edits for manage-data/ingest/transform-enrich/common-mistakes.md
#1
Conversation
| @@ -14,33 +14,33 @@ There are many ways to achieve similar results when creating ingest pipelines, w | |||
| This guide does not provide guidance on optimizing for ingest pipeline performance. | |||
| ::: | |||
|
|
|||
| ## If Statements | |||
| ## Write concise `if` statements | |||
There was a problem hiding this comment.
Use grammatically parallel headings. In this case, I've updated them to all start with a verb.
|
|
||
| ### Avoiding Excessive OR Conditions | ||
| ### Avoid excessive OR conditions |
There was a problem hiding this comment.
Use sentence case in headings.
|
|
||
| ```painless | ||
| ["admin","def", ...].contains(ctx.kubernetes?.container?.name) | ||
| ["admin","def","demo","acme","wonderful"].contains(ctx.kubernetes?.container?.name) |
There was a problem hiding this comment.
Make sure the do and don't examples are actually equivalent for fair comparison.
| :::{tip} | ||
| This example only checks for exact matches. Do not use this approach if you need to check for partial matches. | ||
| ::: |
There was a problem hiding this comment.
Remove unnecessary explanations. I don't think you need to go into detail about checking for partial matches when that's not what the example is trying to illustrate. A quick tip would probably be sufficient. Let me know if you disagree.
| @@ -52,7 +52,7 @@ It is not necessary to use a null safe operator for first level objects | |||
|
|
|||
| For example, if you only want data that has a valid string in a `ctx.openshift.origin.threadId` field: | |||
|
|
|||
| ####  **Don't**: Leave the condition vulnerable to failures and use redundant checks | |||
| ####  **Don't**: Leave the condition vulnerable to failures and use redundant checks | |||
There was a problem hiding this comment.
I removed the alt text for all the check and x icons because they were being rendered in the On this page items, which we don't want.
| ```painless | ||
| "if": "ctx.arbor?.ddos?.subsystem == 'CLI' && ctx.arbor.ddos.command_line != null" | ||
| ctx.arbor?.ddos?.subsystem == 'CLI' && ctx.arbor.ddos.command_line != null <1> | ||
| ``` | ||
|
|
||
| This improves readability and avoids redundant checks. | ||
| 1. Since the `if` condition is evaluated left to right, once `ctx.arbor?.ddos?.subsystem == 'CLI'` passes, you know `ctx.arbor.ddos` exists so you can safely omit the second `?`. |
There was a problem hiding this comment.
Let's use a consistent format for all do and don't code blocks.
|
|
||
| ```painless | ||
| "if": "ctx?.user?.geo?.region != null && ctx?.user?.geo?.region != ''" | ||
| ctx?.user?.geo?.region != null && ctx?.user?.geo?.region != '' |
There was a problem hiding this comment.
Since these are all inside the heading about writing if statements, I don't think we need to include the if in every code block (leaving it out also makes the syntax highlighting better so it's easier to read).
| ::: | ||
|
|
||
| :::{dropdown} Full example |
There was a problem hiding this comment.
This full example feels out of place to me. I added it in a dropdown to prevent it from distracting from the flow of the page, but I'm not sure if that's the right call.
|
|
||
| ## Script processor | ||
| If no built-in processor can achieve your goal, you may need to use a [script processor](elasticsearch://reference/enrich-processor/script-processor.md) in your ingest pipeline. Be sure to write scripts that are clear, concise, and maintainable. |
There was a problem hiding this comment.
I like starting with some context to help the reader quickly decide if a section is relevant to them.
| ```json | ||
| { | ||
| "script": { | ||
| "source": """ | ||
| String timeString = ctx['temp']['duration']; | ||
| ctx['event']['duration'] = Integer.parseInt(timeString.substring(0,2))*360000 + Integer.parseInt(timeString.substring(3,5))*60000 + Integer.parseInt(timeString.substring(6,8))*1000 + Integer.parseInt(timeString.substring(9,12)); | ||
| String timeString = ctx['temp']['duration']; <1> | ||
| ctx['event']['duration'] = Integer.parseInt(timeString.substring(0,2))*360000 + Integer.parseInt(timeString.substring(3,5))*60000 + Integer.parseInt(timeString.substring(6,8))*1000 + Integer.parseInt(timeString.substring(9,12)); <2> <3> <4> | ||
| """, | ||
| "if": "ctx.temp != null && ctx.temp.duration != null" | ||
| "if": "ctx.temp != null && ctx.temp.duration != null" <5> | ||
| } | ||
| } | ||
| ``` | ||
| 1. Avoid accessing fields using square brackets instead of dot notation. | ||
| 2. `ctx['event']['duration']`: Do not attempt to access child properties without ensuring the parent property exists. | ||
| 3. `timeString.substring(0,2)`: Avoid parsing substrings manually instead of leveraging date/time parsing utilities. | ||
| 4. `event.duration` should be in nanoseconds, as expected by ECS, instead of milliseconds. | ||
| 5. Avoid redundant null checks instead of the null safe operator (`?.`). |
There was a problem hiding this comment.
Again, I'm just trying to use a consistent format for all do and don't code blocks.
I'm opening a separate PR so you can review my suggestions with some context. This can be merged into elastic#1381.
👋 @philippkahr