-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Introduce a new flag to explicitly permit legacy monitoring #16586
Introduce a new flag to explicitly permit legacy monitoring #16586
Conversation
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.
Besides the suggestion of renaming the flag, I would suggest to add allow.legacy.monitoring
to env2yaml.go. The main reason is to ease the transition for docker users as they can setup monitoring by just passing the env variable today.
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.
please add the flag to benchmark buildkite 🙏
When I use the xpack.* to enable monitoring in old way, Logstash does not give any hints/ logs that it is missing the flag. I think we need to print logs to guide user.
Let @karenzone to have a look on the words, otherwise, LGTM
One more missing in default_metadata.rb for |
Thanks @kaisecheng for your review, added the allow.legacy.monitoring flag to BK pipeline and now logs a warn when legacy monitoring is enabled and not allowed. Example of line logged:
|
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.
One more missing in default_metadata.rb for
curl localhost:9600/
☝️ Almost there
was fixed in https://github.com/elastic/logstash/pull/16586/files#diff-a0255510e1f939e6ad6e719d7db5ec42a9f0e886171561b1084da93693a44256R6 if I'm not wrong |
@andsel https://github.com/elastic/logstash/blob/main/logstash-core/lib/logstash/api/commands/default_metadata.rb#L73-L76 |
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.
LGTM :)
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 apply only to internal collection? If so, we should call out internal collection specifically. Technically, any collection method we've used in the past would fall under the "legacy" umbrella, and we might need more precision and granularity than would be provided by labeling this one implementation as legacy.
Please LMKWYT.
@karenzone yes it's related just to the internal collector. Do you thing we should switch naming from |
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 think we should consider renaming the setting to live under xpack.monitoring
WDYT?
…ally enable the legacy monitoring collector
…monitoring is overridden
Co-authored-by: kaisecheng <69120390+kaisecheng@users.noreply.github.com>
Co-authored-by: kaisecheng <69120390+kaisecheng@users.noreply.github.com>
… but not allowed.
Co-authored-by: kaisecheng <69120390+kaisecheng@users.noreply.github.com>
…ollection' and moved the registering of such setting from core to xpack
20b2db2
to
c0e0aba
Compare
Co-authored-by: Karen Metts <35154725+karenzone@users.noreply.github.com>
Quality Gate passedIssues Measures |
💚 Build Succeeded
History
cc @andsel |
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.
LGTM
In elastic#16586 the module include was removed. This causes failures in the script when the module is referenced. This commit re-enables the include for the yaml module.
In #16586 the module include was removed. This causes failures in the script when the module is referenced. This commit re-enables the include for the yaml module.
Release notes
Introduce a new flag setting
xpack.monitoring.allow_legacy_collection
which eventually enable the legacy monitoring collector.What does this PR do?
Update the method to test if monitoring is enabled so that consider also
xpack.monitoring.allow_legacy_collection
to determine ifmonitoring.*
settings are valid or not.By default it's
false
, the user has to intentionally enable it to continue to use the legacy monitoring settings.Why is it important/What is the impact to the user?
Permit to the user to force the usage of X-Pack legacy internal collector
Checklist
[ ] I have made corresponding change to the default configuration files (and/or docker env variables)Author's Checklist
How to test this PR locally
elastic
user.config/logstash.yml
), like:config/logstash.yml
with:Related issues
Use cases
As a developer I want to definitively drop the internal legacy collector in favor of ElasticAgent
Logs
Logs without collection override
Logs with collection override