vendure-ecommerce / vendure

The commerce platform with customization in its DNA.
https://www.vendure.io
Other
5.62k stars 995 forks source link

Improve Logger interface to enable tracing #1444

Open michaelbromley opened 2 years ago

michaelbromley commented 2 years ago

Is your feature request related to a problem? Please describe. Currently the Logger interface is rather limited - each method receives a message: string and an optional context: string. The error() method also can take an optional trace: string arg.

For certain applications we want to be able to pass in more data to the logging layer. For example, for tracing a request, there might be a tracing ID set on the express Request object. We'd like to be able to log this in order to view all the logs produced by a particular request as it travels through the API and to the DB.

3rd-party logging/tracing tools like Datadog also want extra data provided.

Describe the solution you'd like The Logger methods should be re-worked to be more extensible. For example:

export interface LogContext<Data = any> {
   // replaces the current "context" string,
   // e.g. "AssetServerPlugin"
  source: string;
  // provide the stack trace even for non-errors
  stack: string;
  ctx?: RequestContext;

}

export interface Logger<Data = any> {

  info(message: string; context: LogContext<Data>): void;
}

To provide the RequestContext object, we'll need to manually pass it to all caller to Logger.<method> in the Vendure code base. We almost always have access to a ctx anyway. This will allow us to access e.g. ctx.req.traceId and pass that ID to our tracing service.

The stack trace can be automatically captured using new Error().stack at the point that the log method is invoked. I looked up the perf implications of newing up an Error object each time and it seems acceptable to do so. Although perhaps there is a clever trick we can do to defer this to only when it is needed, such as by creating a closure function and them providing a getStack() function which only creates an Error object when invoked. Needs testing to see if viable.

Draykee commented 2 years ago

The context should allow additional parameters. Usecase: We want to print logs in a JSON format to make it searchable by properties with kibana.

Logger.info("Message", { ...defaultContext(), user: 1, price: 2000 } );

dlhck commented 1 day ago

Note: We should also add OpenTelemetry support with this one.