Skip to content

Conversation

estolfo
Copy link
Contributor

@estolfo estolfo commented Oct 15, 2025

What does this PR do?

These changes handle the case when config.reload.* settings are set as pipeline override settings in pipeline.yml.
They aren't actually applied at the pipeline level so they are considered deprecated as pipeline override settings; they can only be set at the process level.

More specifically, the changes include:

  • Don't store config.reload.* settings per pipeline, when set in pipeline.yml
  • Remove config.reload.* from pipeline metrics
  • Remove config.reload.* from /_node/pipeline(s) endpoint responses
  • Remove references to config.reload.* from documentation of /_node/pipeline(s) (see preview below in comments)

Why is it important/What is the impact to the user?

See the related issue for more context. In summary, config.reload.* can be set at the pipeline level but the settings are only actually applied at the process level.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files (and/or docker env variables)
  • I have added tests that prove my fix is effective or that my feature works

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

@estolfo estolfo added the bug label Oct 15, 2025
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

Copy link
Contributor

mergify bot commented Oct 15, 2025

This pull request does not have a backport label. Could you fix it @estolfo? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit.
  • If no backport is necessary, please add the backport-skip label

@estolfo
Copy link
Contributor Author

estolfo commented Oct 15, 2025

Some questions/comments about these changes:

  1. When a config.reload.* setting is attempted to be set via #merge_pipeline_settings, it warns that it’s deprecated and doesn’t actually set it.
  2. Should the Settings#to_hash method also remove config.reload.* settings from the hash that is returned? If I'm not misreading the code, the Settings object is also used to track process-level settings, which could include config.reload.*. So Settings#to_hash shouldn't remove config.reload.*. The settings are not actually set as described in 1) so it shouldn't be necessarily to filter them from Settings#to_hash's response.
  3. I didn't find any documentation showing that config.reload.* are pipeline-level options so there are no docs changes in this PR. I updated documentation for the _node/pipelines endpoint showing that the config.reload.* options were included in the response.
  4. It's not yet clear to me whether removing these lines would be a breaking change. It seems that the two options are included in the response of the api endpoint and perhaps that would be a breaking change, depending on how that endpoint is used. As per the documentation, the config.reload.* settings are included in the response from /_node/pipeline(s) as pipeline level settings. So I've updated the endpoint to not include those settings in the response.

@estolfo
Copy link
Contributor Author

estolfo commented Oct 17, 2025

preview /_node/pipelines documentation with the changes to logstash.yml
image

and /_node/pipelines/{pipeline_name}

image

@estolfo estolfo marked this pull request as ready for review October 17, 2025 13:34
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@kaisecheng kaisecheng self-requested a review October 20, 2025 16:33
Copy link
Contributor

@kaisecheng kaisecheng left a comment

Choose a reason for hiding this comment

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

Overall looks good. Docs and API spec are updated. When Logstash is started with auto reload "-r", the deprecation log by default is printed every 10 seconds. I think it is acceptable.

I have two minor suggestions.

hash.each do |key, _|
unless PIPELINE_SETTINGS_WHITE_LIST.include?(key)
if DEPRECATED_PIPELINE_OVERRIDE_SETTINGS.include?(key)
deprecation_logger.deprecated("Config option (#{key}) is deprecated as a pipeline override setting, please only set it at the process level.")
Copy link
Contributor

Choose a reason for hiding this comment

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

I have two suggestions

  • quote the key with "`" rather than "("
  • Can we add the pipeline name to the log? Sometimes users have hundreds of pipelines, so it will be great to point them to the right place.

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