search

elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.73k stars 8.14k forks source link

Add execution context to log records #60122

Open joshdover opened 4 years ago

joshdover commented 4 years ago

In order to support enhanced observability of Kibana itself, we should add a richer "execution context" to the New Platform's logging service that allows correlation between different log records that occur during an "execution context" such as a request or background task.

This context should automatically be attached to any log records produced during an execution context, ideally by leveraging the RequestHandlerContext pattern.

Fields that should be part of this context:

Plugins should be able to leverage this identifier to find other useful information about these log events, such as the current user.

Related issue: #52125

elasticmachine commented 4 years ago

Pinging @elastic/kibana-platform (Team:Platform)

pgayvallet commented 4 years ago

Do we have an idea how we would 'bind' the logger's records to the request execution context in a node environment without tools such as Java's threadlocal ?

joshdover commented 4 years ago

@thomheymann is this a blocker for security anymore? As I understand it, this only affects regular log messages and should not affect the audit trail logs at all.

While I agree that adding the request ID to regular logs is very useful, I think we are fine if we delay this past 7.10.

pgayvallet commented 2 months ago

@elastic/kibana-security do you knoe if that is still a requierement on your side?

legrego commented 2 months ago

@elastic/kibana-security do you knoe if that is still a requierement on your side?

@pgayvallet I'm not sure what the original goal was. Our audit logs already include a trace id to understand everything that happens within the context of a specific KibanaRequest.

If we're looking for something that spans multiple requests (such as the loading of a dashboard), then we don't have a mechanism for that, yet. I think this would be a welcome addition to our audit events, but this isn't something we are currently prioritizing.

pgayvallet commented 2 months ago

I'm not sure what the original goal was. Our audit logs already include a trace id to understand everything that happens within the context of a specific KibanaRequest.

The idea is to be able to attach a traceId to all our log entries, so that we can track execution of a specific request more easily (so basically what you did for audit logging, but for all our logging system). This is similar to log4js' MDC.

rudolf commented 2 months ago

It surprises me that we don't have a logger on the RequestHandlerContext that would be a natural place to enrich the logger with the request context.

The most frequent logging pattern in routes is probably to log on error, so it seems like we could perhaps get a lot of the value by just enriching our 500 / error logger with the trace id similar to additional fields we added in https://github.com/elastic/kibana/pull/169552

pgayvallet commented 3 weeks ago

Yeah, I agree that just exposing a scoped logger via the request handler context would already do a lot, while being way less complex technically (no need for async context or anything).

The downside of the approach would be that we need all route handlers to change their code to use this RHC's scoped logger, where a more complex approach (such as async context) might be able to do it automatically without any code change in any handler.