-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
async-import plugins in the server side #170856
Conversation
@opauloh, that's a great idea! I've created #171080 to tackle it since it looks to me like an effort on its own 😇 |
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.
Response Ops changes LGTM. Code review only
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.
deployment management changes 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.
Thanx for your work here Alejandro! Visualizations team numerous plugins changes 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.
ResponseOps changes 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.
Kb Security changes 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.
DE changes 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.
Logstash changes 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.
Desk tested security solution and timelines. Both working well. 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.
Looks great, thanks @afharo !
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 for Protections Experience team :)
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Public APIs missing comments
History
To update your PR or re-run it, just comment with: cc @afharo |
Summary
When Kibana starts, it has to import the file
${plugin}/server/index
for all the plugins in order to read theconfig
variable that it's exported from that file.Since the
index
file for each plugin loads the./plugin
file, it's indirectly loading all the files of the plugin in one go. This essentially makes Kibana load all the server-side code from all the plugins, even when the plugins might be disabled. While it wasn't a big deal in the Classic offering (most plugins are enabled), on Serverless we are disabling a lot of plugins (and also adding a good amount of Serverless-specific plugins that shouldn't load on Classic).This PR changes all the
${plugin}/server/index
files to follow a pattern like:Checklist
Risk Matrix
unavailable
status before it is actually serving any HTTP traffic. Commit 0404c34 fixed the FTR, but that means we might expect things to slightly change the order in which they occur when Kibana is starting. This should only affect automations that depend on specific log entries because Kibana won't reportavailable
until all plugins are correctly setup.For maintainers