-
Notifications
You must be signed in to change notification settings - Fork 197
Painless docs overhaul (troubleshooting) #3649
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
base: main
Are you sure you want to change the base?
Conversation
Vale Linting ResultsSummary: 4 suggestions found 💡 Suggestions (4)
The Vale linter checks documentation changes against the Elastic Docs style guide. To use Vale locally or report issues, refer to Elastic style guide for Vale. |
yetanothertw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi David,
The content looks good so far, my comments are mostly focused around the structure (sections) of the troubleshooting guide.
I only managed to review the first troubleshooting page this morning. I'll keep going and will be adding more comments.
troubleshoot/elasticsearch/painless-array-list-manipulation-errors.md
Outdated
Show resolved
Hide resolved
| } | ||
| ``` | ||
|
|
||
| ## Root cause |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering about this structure. The info in this Root cause section is kind of redundant (it repeats the same info as the topic description, so I'm questioning its user value 🫣)
I only skimmed through some of the existing Troubleshoot > Elasticsearch topics, and I don't think they follow a specific pattern. I do like the idea that a template brings consistency across topics, but it stands out against the existing content -- not sure what the best strategy would be (new topics using a new format/structure or follow existing patterns). I know our team is working on developing guidance and templates for troubleshooting topics (check out this PR it also includes a new template for troubleshooting topics).
For consistency, I think the content would be better aligned with the docs team efforts -- this guidance makes more sense to me. Btw it also recommends that we do not explain causes, so you can safely get rid of this section.
|
|
||
| The error occurs because the script tries to access index 2 (the third element) in an array that only has two elements (indices 0, 1). Arrays in Painless are zero-indexed, so accessing an index greater than or equal to the array size causes an exception. | ||
|
|
||
| ## Solution: Check array bounds before accessing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should Solution be substituted with Resolution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's because I don't know Painless at all, but I would find callouts useful to orient the reader, what should they be paying attention to?
| } | ||
| ``` | ||
|
|
||
| ## Notes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This content should be included higher up in the page ("before the fold"), perhaps incorporated into the page description and/or also in the Resolution section.
| - id: elasticsearch | ||
| --- | ||
|
|
||
| # Troubleshoot array manipulation errors in Painless |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this title is too generic. It could be a bit more specific and focused on the issue as its experienced from user the point of view, perhaps something along the lines of:
Accessing an index greater than or equal to the array size causes an exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking about titles such as these, for example:
- Warning: Not enough nodes to allocate all shard replicas
- Total number of shards for an index on a single node exceeded
- Troubleshoot incomplete migration to data tiers
It would also be better aligned with the WIP guidance:
| } | ||
| ``` | ||
|
|
||
| ## Problematic code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm questioning the usefulness of this section as well, it goes into overly-explaining territory.
If you want to keep the code, maybe hide it in a dropdown so it doesn't make the page look too bloated? Another thing is the title problematic code sounds like attributing blame and I'm sure that's not the intention at all. It's tricky to find the right balance between trying to help the user without making them feel like they did something wrong (even if technically we're addressing errors in this section).
| } | ||
| ``` | ||
|
|
||
| ## Sample document |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like it's a part of the solution, right? It's an example that illustrates (the Results section) the solution, so maybe it should be included in the Solution/Resolution section?
yetanothertw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part 2 of comments. Although they're very similar to the first batch. I think the majority of it is a structure mismatch with other existing content, and best practices guidance that's being worked on.
|
|
||
| # Troubleshoot date math errors in Painless | ||
|
|
||
| Follow these guidelines to avoid [date](elasticsearch://reference/scripting-languages/painless/using-datetime-in-painless.md) operation errors in your Painless scripts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of the structure-related comments apply to this topic as well. I think information in the Solution and Notes sections should be brought into the introduction. You can also summarise why (the root cause) this runtime error occurs.
The error message is more important than the problematic code (which is so similar to the solution it feels like a duplication). I think the solution code example with its explanation is enough.
| - id: elasticsearch | ||
| --- | ||
|
|
||
| # Troubleshoot date math errors in Painless |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The title would be more useful if it was rewritten with the error message in mind, maybe something like illegal_argument_exception when calling date methods without using .value or Fixing illegal_argument_exception in scripts by accessing date fields via .value
| } | ||
| ``` | ||
|
|
||
| ## Root cause |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly to other pages, I'd integrate this information into the page description, and the same with the Notes section.
|
|
||
| For example, calling `doc['author_score'].value` directly on a document that does not contain that field causes an error. The recommended approach is to use `doc[<field>].size()==0` to check if the field is missing in a document before accessing its value. | ||
|
|
||
| ## Sample documents |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the various solutions based on these examples? If you really need to keep the examples, maybe they can be collapsed?
Or perhaps they're better suited in a contextual type of a doc where the aim is to educate users?
| } | ||
| ``` | ||
|
|
||
| ## Results |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this example refer to all three solutions? (do they all converge into the same result?) If so, you can just make this codeblock part of the solutions section.
| } | ||
| ``` | ||
|
|
||
| ## Notes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This information is repetitive at the end of the page. I'd find ways to add into the problem or solution description.
| - id: elasticsearch | ||
| --- | ||
|
|
||
| # Troubleshoot ingest pipeline failures in Painless |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something more specific could work, like Troubleshoot ingest pipeline failures in arithmetic operations in Painless
| } | ||
| ``` | ||
|
|
||
| ## Problematic code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would reframe this as instead of using ctx.nanoseconds = ctx.time_field * 1000000; use ctx.timestamp_nanos = millis * 1000000L; or something like that. I'd mention that in the Solution section.
|
|
||
| ## Root cause | ||
|
|
||
| When accessing fields using `ctx.time_field` in ingest pipelines, the values are not automatically parsed to their mapped field types. The script attempts to multiply a string value (time field) directly with an integer. Time strings such as `"00:00:00.022"` remain as strings. They need to be properly parsed as dates and converted to epoch milliseconds before performing arithmetic operations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be part of the intro. The same as the Notes section.
…rors.md Co-authored-by: Vlada Chirmicci <vlada.chirmicci@elastic.co>
This adds a new "Painless scripting" section into the Troubleshoot > Elasticsearch docs, based on the Painless doc project with the Kibernum team.
All changes are in this Troubleshoot Painless scripting in Elasticsearch section.
Related: