-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Allow plugins to extend logging config #61976
Comments
Pinging @elastic/kibana-platform (Team:Platform) |
That mean that a plugin should be allowed to update anything included in the |
Thinking about the API design here a bit, my current plan to is to change the export interface PluginInitializerContext<ConfigSchema = unknown> {
opaqueId: PluginOpaqueId;
env: {
mode: EnvironmentMode;
packageInfo: Readonly<PackageInfo>;
};
+ logging: {
+ logger: LoggerFactory;
+ configure(config$: Observable<LoggerConfig>): void;
+ };
- logger: LoggerFactory;
config: {
legacy: { globalConfig$: Observable<SharedGlobalConfig> };
create: <T = ConfigSchema>() => Observable<T>;
createIfExists: <T = ConfigSchema>() => Observable<T | undefined>;
};
} Of course this will be a breaking change, though it should be trivial. If desired, we could keep the existing The interface LoggerConfig {
appenders: Map<string, AppendersType>;
loggers: Array<{
appenders: string[];
context: string;
level: 'all' | 'fatal' | 'error' | 'warn' | 'info' | 'debug' | 'trace' | 'off';
}>;
} One limitation is that the
I lean towards (1) on this due to the simplicity, though it does introduce a "caveat" or "gotcha" in the consumption of this API. Another question is how to deal with the user-defined configuration and the plugin-defined configuration, potential options:
I lean towards option (1) for the simplicity and explicitness. |
Could there be any cases when the logging system emits a logging record before the plugin initialized and adjusts the config? In this case, we have to configure the logging context in a declarative manner via static exports. logging: {
logger: LoggerFactory;
configure(config$: Observable<LoggerConfig>): void;
}; What are the benefits of using
I'd expect that the user's configuration always takes precedence. However, a plugin can provide its own configuration to adjust the logging format.
Wouldn't they exist in the same setup(core){
const loggingConfig$ = this.config$.pipe(map(cfg => {
return { layout: { pattern: cfg.pattern || '{date} {level}'...
}));
core.logging.configure(loggingConfig$); |
As a plugin's context is already implicitly prepended by
+1 on that. Main argument would be that if the user explicitly configured a silent or very verbose logging, plugins shouldn't be able to override it.
We could expose helper APIs to help them, as webpack is doing with |
Core does have access to a plugin's log context, so it is possible, but we have control over that. Right now, the only messages I am aware of in Core are the lifecycle debug logs. However, these are currently only emitted after the plugin constructor has been created. This was one of my motivations for including this API in the PluginInitializerContext. The other was that config is also exposed in this API, and I suspect will drive the customizations that a plugin will be doing to it's logging config.
I think maybe we're talking about different parts of the config here. For extra clarity, I see two options of where the user can configure this logging context: in the main logging config, or in the config of the audit logging plugin.
logging:
appenders:
custom_audit_log:
kind: file
path: /path/to/audit.log
layout:
kind: json
loggers:
- context: plugins.audit_logger
appenders: [custom_audit_log]
level: 'all'
xpack.audit_logger:
appenders:
custom_audit_log:
kind: file
path: /path/to/audit.log
layout:
kind: json If we went with option (1) then This is probably fine, but the audit logger may need to also expose a |
I'd suggest removing those logs. It looks redundant since
Having removed them, we can switch to the regular API exposed via setup contract.
This option is closer to the regular logging configuration, so 👍 . However, I'm concerned that the user config doesn't override the plugin config completely. For example, a user provides a customized config, and then in logs they will find 2 different files containing the same records in different formats. It sounds like a permanent source of confusion. |
It's not a requirement and not in scope for the MVP of the audit logging overhaul. However, it would be nice to eventually be able to modify the audit logging config from the UI and REST API. Elasticsearch's audit logging can be configured via REST API today, and we'll want Kibana to eventually reach parity with it, I assume. Perhaps @arisonl has something else to add. With regard to @joshdover's proposals on how to configure audit logging -- option (1) or option (2) -- I lean towards option (1) as well. We will need to expose additional configuration settings that aren't generic logging settings, so it makes sense to me to have all of the audit logging settings exposed by the plugin so they're in the same space (with the same prefix). |
Agreed 👍
@jportner If I understand correctly, this is what option 2 does, not option 1. FWIW, I think option 2 is easier to implement correctly and possibly easier for the user to understand. The primary downside is that audit logging will be configured in a different part of the yml file from all other logging. But all of the audit logging options will be in the same place rather than split between the global logging config and the audit logging config. Option 2 seems like the better tradeoff to me: easier to implement correctly + keeps all the config for audit logging in the same place (however it is separate from the rest of logging config). |
Quick note in support of Joe's vision, we are ultimately aiming to create a unified audit logging capability between ES and Kibana, one which users are able to find the information they are looking for, easily in one place. UX is very important. So we would definitely want the ability to config through API and UI in the future, beyond the MVP. |
Another disadvantage of this approach is a divergence from elasticsearch audit logging configuration, which it is closer to the 1st option https://github.com/elastic/elasticsearch/blob/master/x-pack/plugin/core/src/main/config/log4j2.properties |
@joshdover sorry, I was trying to digest all of the earlier comments and I think I got my numbers mixed up with what was said before. But yes, re-reading this, I intended to say option 2.
@restrry good catch... there's no mention of those settings on the Elasticsearch Auditing Settings docs page, either. I appreciate sticking with convention most of the time, but I feel this is one situation where it makes sense to diverge. Having audit logging settings in two different places is a pretty confusing/bad UX. So, I'll reiterate my vote of support for option 2! |
Agreed, and I also think implementing the API-based configuration will be easier if the config is completely driven by the plugin rather than merged with the user-defined config. With this option, we should at least emit a warning log if the user does define config in the @arisonl is consistency between ES and Kibana on the config critical here? If so, who on the ES side could we talk to about getting alignment on this? If not, and there are no "fundamental flaw" objections, I will proceed with implementing option 2. |
Allow plugins to dynamically modify the logging config for the contexts that they own.
Blocker for Audit Logging (#52125)
The text was updated successfully, but these errors were encountered: