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

Expand/fix custom presets docs #870

Merged
merged 4 commits into from
Jan 31, 2024
Merged

Conversation

kilfoyle
Copy link
Contributor

@kilfoyle kilfoyle commented Jan 30, 2024

This PR updates the performance tuning presets sections of the Elasticsearch output docs:

  • Updates some of the setting terms (e.g., compression to compression_level
  • Adds detail about customizing settings.

Rel: #869

Please see these example preview pages:

Here's the updated section:

Screenshot 2024-01-30 at 10 55 09 AM

@kilfoyle kilfoyle requested a review from a team as a code owner January 30, 2024 16:02
Copy link

A documentation preview will be available soon.

Help us out by validating the Buildkite preview and reporting issues here.
Please also be sure to double check all images to ensure they are correct in the preview.

Request a new doc build by commenting
  • Rebuild this PR: run docs-build
  • Rebuild this PR and all Elastic docs: run docs-build rebuild

run docs-build is much faster than run docs-build rebuild. A rebuild should only be needed in rare situations.

If your PR continues to fail for an unknown reason, the doc build pipeline may be broken. Elastic employees can check the pipeline status here.

Comment on lines 107 to 108
flush.min_events: 1600
flush.timeout: 10
Copy link
Member

Choose a reason for hiding this comment

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

These need the queue.mem. prefix per elastic/kibana#175892

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @kpollich. I've pushed the change here and at line 231 below.

I'm a bit confused though. Would you know if these settings descriptions for standalone agent also need to be updated? We have queue memory settings described for Elasticsearch, Logstash, and Kafka (currently in an open PR).

Those pages all show an example configuration like this:

queue.mem:
  events: 4096
  flush.min_events: 512
  flush.timeout: 5s

Copy link
Member

Choose a reason for hiding this comment

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

The "expanded" YAML syntax is equivalent, but perhaps we should be consistent, e.g.

queue.mem.events: 4096
queue.mem.flush.min_events: 512
queue.mem.flush.timeout: 5s

is equivalent to

queue:
  mem:
    events: 4096
    flush:
      min_events: 512
      timeout: 5s

which is also equivalent to

queue.mem:
  events: 4096
  flush.min_events: 512
  flush.timeout: 5s

In YAML, dot notation or indent notation are both valid. It's largely subjective where one uses dots and where one introduces a new level of indentation. I'm in favor of aligning these example code blocks with what's in those other preexisting docs if only because it'd be less work than updating those other docs to match the way we have it in these ingest docs 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Brilliiant! Thanks @kpollich for the super clear explanation. I wasn't sure and certainly didn't want to just guess. :-)

I've updated all instances of the events, flush.min_events, and flush.timeout settings to be queue.mem.events, queue.mem.flush.min_events, and queue.mem.flush.timeout, respectively.

The configuration examples in the standalone agent output settings docs are now like this:

  queue.mem.events: 4096
  queue.mem.flush.min_events: 512
  queue.mem.flush.timeout: 5s

The docs preview linked in the description will update once the CI checks have re-run.

Copy link
Member

@kpollich kpollich left a comment

Choose a reason for hiding this comment

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

🚀 Awesome work as always

@kilfoyle kilfoyle merged commit a77df9a into elastic:main Jan 31, 2024
4 checks passed
mergify bot pushed a commit that referenced this pull request Jan 31, 2024
* Expand/fix custom presets docs

* Add queue.mem. prefix

* Add 's' for the timeout settings example yaml configs

* Update all memory queue settings with 'queue.mem.' prefix; fix yaml examples

(cherry picked from commit a77df9a)
kilfoyle added a commit that referenced this pull request Jan 31, 2024
* Expand/fix custom presets docs

* Add queue.mem. prefix

* Add 's' for the timeout settings example yaml configs

* Update all memory queue settings with 'queue.mem.' prefix; fix yaml examples

(cherry picked from commit a77df9a)

Co-authored-by: David Kilfoyle <41695641+kilfoyle@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants