Make core logging independent from the legacy Kibana.#21831
Make core logging independent from the legacy Kibana.#21831azasypkin merged 2 commits intoelastic:masterfrom
Conversation
| static async create(opts = {}, settings = {}) { | ||
| const transformedSettings = transformDeprecations(settings); | ||
| const config = await Config.withDefaultSchema(transformedSettings); | ||
| const config = Config.withDefaultSchema(transformedSettings); |
There was a problem hiding this comment.
note: I don't see any reason to make async that is really sync :) But the main reason for this change is that with async it's hard to reliably use Config.withDefaultSchema inside constructors.
There was a problem hiding this comment.
I seem to remember that it was a part of an initial attempt to support loading the package.json files from plugins without executing their code, but based on the current state of #18926 it doesn't seem necessary
There was a problem hiding this comment.
Interesting! Now I'm curious what was supposed to be async inside of withDefaultSchema @kobelb ? :)
There was a problem hiding this comment.
It's because of this: https://github.com/elastic/kibana/pull/17496/files#diff-346d12eb90facc1036ff15f93bd825bf
To be able to determine if the system call filters should be enabled by default, we have to manually check the OS version here, which is async: https://github.com/elastic/kibana/pull/17496/files#diff-12472d028fb5d43810d33bc6d77e93c4
There was a problem hiding this comment.
That makes sense, there have been a few other places where I've resorted to using the async default configs as well: https://github.com/elastic/kibana/blob/master/x-pack/plugins/reporting/index.js#L91 https://github.com/elastic/kibana/blob/master/x-pack/plugins/reporting/index.js#L98
There was a problem hiding this comment.
Hmm, thanks for sharing! You have quite a few smart defaults :)
There was a problem hiding this comment.
I somehow found myself in a position where I needed quite a few different defaults for different operating systems, with a really poor way of detecting support....
There was a problem hiding this comment.
One way we can handle this sort of thing in the new platform would be to construct an Environment object before other services that we can pass around to APIs like this. This way by the time something like this would need to determine the operating system, that value would have already been determined and made available synchronously through the Environment.
There was a problem hiding this comment.
We do exactly that (almost) with env and installationFeatures (isClusterModeSupported, isOssModeSupported, isXPackInstalled etc.) in new platform: https://github.com/elastic/kibana/pull/19994/files#diff-806634e0e6f42d8ea73cd3565c9089cbR31 . Env can include other environment specific things for sure.
| } | ||
|
|
||
| /** | ||
| * The "legacy" Kibana uses Hapi server + even-better plugin to log, so we should |
There was a problem hiding this comment.
note: I can theoretically just use hapi (v14) instance here, but as I mentioned in comment it feels a bit too much to me (and it's going to be upgraded soon as well), but let me know if you'd rather prefer something else here.
| constructor(legacyLoggingConfig: Readonly<Record<string, any>>) { | ||
| super(); | ||
|
|
||
| // We set `ops.interval` to max allowed number and `ops` filter to value |
There was a problem hiding this comment.
note: I dug through source code of even-better and couldn't find a better way to just disable ops monitoring :/
💚 Build Succeeded |
|
Seems we have several reviewers, so removing review request. Please add me again if needed, though. |
💚 Build Succeeded |
💚 Build Succeeded |
spalger
left a comment
There was a problem hiding this comment.
Pulled the PR, seems to run well, and the code looks good, LGTM!
* Make core logging independent from the legacy Kibana. (#21831) * Remove `async` from test `setup` function.
|
6.x/6.5: 402758f |
In this PR I decouple core logging from the "legacy"
kbnServer.server.logso that core can log before "legacy" Kibana is started or not started at all (e.g.ClusterManagerprocess). It also eliminates the need in passingenvthat deep in core services/system hierarchy (envshould have been passed toAppendersfactory).Surely it may look a bit hackish, but this logging backward compatibility bridge is supposed to die as soon as we switch entire Kibana to use the core own logging system.
Unblocks #19994