feat(extension-logging): add integration with winston and fluentd logging

[raymondfeng]

Experimental feature for #4089

See docs at https://github.com/strongloop/loopback-next/blob/logging/extensions/logging/README.md.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

• npm test passes on your machine
• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide
• API Documentation in code was updated
• Documentation in /docs/site was updated
• Affected artifact templates in packages/cli were updated
• Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

[bajtos]

I'd like to start the discussion with building a better understanding of different loggers and log collectors that we can choose from.

@raymondfeng Why did you pick winston and fluentd in particular?

Personally, I prefer to use pino for logging, it's more modern than winston/bunyan and is significantly more performant (2x faster according to pino's benchmarks).

I am not familiar with log collectors. I see that fluentd is a part of Cloud Native Computing Foundation, I guess that makes it a good candidate.

@strongloop/loopback-next thoughts?

[bajtos]

Please don't start Docker containers from the tests. It slows down the test suite that's already too slow. If our tests need an external service to be running, then please provide a script that the developers can run ONCE to start all services at the beginning of their coding session. See what we have already in place in acceptance/repository-* directories and in individual connectors like loopback-connector-mysql.

As we discussed when working on the monitoring/Prometheus integration, it's often a good practice to mock the external dependency (i.e. the log collector) for integration tests to make them faster and then write only few acceptance/end-to-end tests that are talking to the real service started in Docker.

[bajtos]

Also, is it a good idea to have our component coupled to Winston and Fluent? The tricky part is to strike the good balance between making the framework easy to use by prescribing the recommended way, but stay flexible enough to support different use cases.

I think it should be ok to hard-code the logging framework we use under the hood, we can be prescriptive here. On the other hand, I'd like the log outputs (transports) to be as flexible as possible - ideally our users should have the option to choose any transport they like, and have an easy (and well-documented) way how to configure this aspect of logging.

Typically, I would like to print my logs to stdout (console) while running the service under npm test. When running the service locally on my developer machine, I would ideally like to see the logs in my console too, but I am not sure if that's feasible when running the service via Docker or in a Kubernetes cluster.

[raymondfeng]

The PoC starts with Fluentd and Winston for a few reasons:

1. Fluentd is part of CNCF - which aligns with our Cloud Native integration direction, such as health endpoints for k8s, metrics for prometheus, and distributed tracing with jaeger.

2. Winston is a mature logging framework in Node and it has the key extensibility such as formatters and transports. We leverage them to enable the extension point.

3. Fluentd Node.js client happens to have a transport for Winston.

4. I was peeking Pino as well.

I didn't want to start our abstraction for the common logging building blocks. That's why I picked popular frameworks to test drive.

[bajtos]

I quickly skimmed through the new version.

My biggest objection is against the decorator name log, I find it as too generic. It can log invocation arguments, produce a HTTP request log or do something else, the name does not provide any clues.

The rest of the comments are of lesser importance, some of them could be addressed later.

[bajtos]

I have mixed feelings about the design where we call this.component() in one place and then app.configure in another. Personally, I would expect to find the configuration in the same place where the component is mounted, i.e.

this.component(LoggingComponent, {
   enableFluent: false, // default to true
   enableHttpAccessLog: true, // default to true
});

Implementation wise, I can imagine this.component calling app.configure under the hood.

The biggest issue I see is how to add type safety, to let the compiler understand what configuration options the component allows. IIUC, this problem is not solved for app.configure either, therefore it's out of scope of this pull request. I just want to point it out.

@strongloop/loopback-next thoughts?

On the second thought, I think there is even more important aspect we should consider: which part of the configuration are specific to deployment environment (dev/staging/production) and which are wiring different code bits together (thing of DI)? The former is typically provided from outside of the application, for example consider the port where the REST server will listen on.

In this case, I think both enableFluent and enableHttpAccessLog option are affecting which code bits are mixed into the application, therefore they should be hard-coded by the app, similarly to how we are hard-coding dependency wirings.

On the other hand, I feel the configuration of the transport and possibly logger is something to configure from outside of the app, depending on the deployment environment.

Anyhow, I guess this can be left out of scope of this initial pull request, because the logging extension is clearly labelled as experimental.

[bajtos]

I agree a short name would be better. We need to find a right balance between a name that's too generic/not enough descriptive (log) and a name that's descriptive but too long (logInvocationArguments).

How about logArgs?

[raymondfeng]

@bajtos I renamed @log to @logInvocation - to log invocation for the decorated method, including args, return value, and errors.

[raymondfeng]

@bajtos PTAL

[bajtos]

Looks mostly good, I'd like somebody else from the team to take over the review process of this pull request and ownership of the new packages.

Few comments:

• I feel the README is not clear enough, not all of my questions in feat(extension-logging): add integration with winston and fluentd logging #4117 (comment) were answered AFAICT

• You forgot to register the new packages in our internal documentation, see https://github.com/strongloop/loopback-next/blob/75ba77d5bdfe24f612c418c9100fa7e008211709/docs/site/DEVELOPING.md#register-the-new-package:

  Please register the new package in the following files:

  • Update MONOREPO.md - insert a new table row to describe the new package, please keep the rows sorted by package name.
  • Update Reserved-binding-keys.md - add a link to the apidocs on Binding Keys if the new package has any.
  • Update CODEOWNERS - add a new entry listing the primary maintainers (owners) of the new package.

[emonddr]

Cool stuff, @raymondfeng .

Please fix typo in test folder name: