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

Doc: Add json_lines known issue to release notes #16831

Draft
wants to merge 6 commits into
base: 8.17
Choose a base branch
from

Conversation

karenzone
Copy link
Contributor

@karenzone karenzone commented Dec 20, 2024

@karenzone karenzone added the docs label Dec 20, 2024
@karenzone karenzone requested review from yaauie and robbavey December 20, 2024 00:24
@karenzone karenzone self-assigned this Dec 20, 2024
@karenzone
Copy link
Contributor Author

karenzone commented Dec 20, 2024

@robbavey @yaauie, I'm getting this draft up so that you can see what you think about the content and presentation. Meanwhile, I'll continue to work on this, adding links where appropriate, etc.

You'll notice that I reworked the existing known issue, too, so please keep me honest that I got everything right.

I know that this content needs to go other places in this branch. Let's get the content right, and then I'll add it to other sections before final signoff

**Solution:** Apply the default change contained in the newer 'jvm.options' file, as seen in this https://github.com/elastic/logstash/blob/main/config/jvm.options#L66-L82[example].

[[known-issue-8-17-0-json_lines]]
===== Error with `json_lines` codec 3.2.0 (or earlier) and {ls} 8.17.1 (or earlier)
Copy link
Contributor

@mashhurs mashhurs Dec 20, 2024

Choose a reason for hiding this comment

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

Suggested change
===== Error with `json_lines` codec 3.2.0 (or earlier) and {ls} 8.17.1 (or earlier)
===== Error with `json_lines` codec 3.2.0 (or later) with {ls} 8.17.0

and {ls} 8.17.1 (or earlier) - part is a bit confusing to me. I support removing LS version from this title which brings reader to "... present in {ls} versions 8.16.0, 8.16.1, and 8.17.0." and to mitigation section.

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 was based on description in issue: "When using the json_lines codec > 3.2.0 with versions of Logstash < 8.17.1, the following error may be emitted, crashing the pipelines"

If that's not accurate, I'll update it here, and it will also need to be updated in the issue.

Copy link
Member

@robbavey robbavey Dec 20, 2024

Choose a reason for hiding this comment

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

What do you think about using the symptom as the heading, rather than json_lines codec?

===== "Input Buffer Full" error With Logstash `8.17.0`, `8.16.1` and `8.16.0`

Then describe the instances of what happens when this error occurs, how this would happen and how to mitigate, with Logstash->Logstash communication first, as it is the most likely, then the json_lines case?

Leading with json_lines isn't necessarily obvious as the codec is used "silently" by the Logstash integration plugin.

**Solution:** Apply the default change contained in the newer 'jvm.options' file, as seen in this https://github.com/elastic/logstash/blob/main/config/jvm.options#L66-L82[example].

[[known-issue-8-17-0-json_lines]]
===== Error with `json_lines` codec 3.2.0 (or earlier) and {ls} 8.17.1 (or earlier)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
===== Error with `json_lines` codec 3.2.0 (or earlier) and {ls} 8.17.1 (or earlier)
===== Error with `json_lines` codec 3.2.0 (or later) and {ls} 8.17.1 (or earlier)

Based on description in the issue, I think this should be 3.2.0 or later for json_lines?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, json_lines codec >= 3.2.0 versions which have an ability to set the buffer limit.

@karenzone
Copy link
Contributor Author

Please make sure that I understood and implemented your suggestions correctly, @robbavey and @mashhurs

Copy link
Member

@robbavey robbavey left a comment

Choose a reason for hiding this comment

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

Very close!

docs/static/releasenotes.asciidoc Outdated Show resolved Hide resolved
==== Known issue

[[known-issue-8-16-1-json_lines]]
===== "Input buffer full" error with {ls} 8.16.0, 8.16.1, or 8.17.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems kinda weird to have 8.17.0 listed in the 8.16.1 and 8.16.0 release notes.
However, this information is complete and correct, and I'm inclined to leave it. LMKWYT.

Note to self @karenzone: Make sure any change here gets propagated to 8.16.1 and 8.16.0 sections.

Copy link
Member

Choose a reason for hiding this comment

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

It does, and it's not something we typically do.

However, the fact that this is not affected in 8.16.2 may make this appropriate to be explicit about which versions are affected to ensure that users are not confused, and don't think we have just missed 8.16.2.

It's something that could affect us in the future, as we support multiple branches, and the bug story isn't as simple as it used to be, so it's probably up for debate how we want to approach this from a messaging standpoint.

Copy link
Contributor Author

@karenzone karenzone left a comment

Choose a reason for hiding this comment

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

Left some comments/questions inline.

If you have any thoughts of preferences on which order we should list known issues for each release, please make them known.

@karenzone
Copy link
Contributor Author

Pending decisions and approvals. Ping me when we're ready to move on this again.

Copy link
Contributor

@mashhurs mashhurs left a comment

Choose a reason for hiding this comment

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

Changes make sense to me!
Thank you alot @karenzone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants