[FEATURE REQUEST] Provide context per class

[maricn]

Is your feature request related to a problem? Please describe.

I'm using globally imported pino logger module. Every time when I make a log line I would like to know from which class the log was created. Right now, I have to include the context manually. Like the following lines in the README.md:
https://github.com/iamolegga/nestjs-pino/blob/master/README.md#L56
https://github.com/iamolegga/nestjs-pino/blob/master/README.md#L72

Currently, we can set base context per configuration of logger module. That's too broad for my use case. Even if I migrate my logger module import from global to per module import, I'd prefer finer level of granularity in defining base context. (But not as fine as log line :) )

Describe the solution you'd like

I would like to write log lines using injected logger without manually providing class context on each log line. I'd like the context to be either set up once per class (injection) or somehow dynamically configured to use class name (or whatever else) per module configuration.

I'm probably off on these, but my superficial suggestions would be:

• configure the module with an option to always fork a child logger for each class it's injected into OR
• expose child functionality so we can fork (child) a logger manually. Then we could set it up with context in each class constructor (or wherever we deem useful)

[iamolegga]

Hi, I think it can be solved with changing injection scope to TRANSIENT.

And then to save compatibility with built in logger we will left arguments as they are for now, but if you want to pass interpolation values to message string, you could just pass undefined or null to context argument, example:

class MyClass {
  // ideally context will be set automatically as MyClass.name, I'll check this out
  // if not we will set it in constructor something like `this.logger.context = 'some string'`
  constructor(@Inject() private readonly logger: Logger) {}

  doSmthng(arg: any) {
    // here we just pass `undefined` to let it be overwritten underhood
    this.logger.log(`arg: %o`, undefined, arg)
  }
}

@maricn Is this API will be ok to use?

[maricn]

@iamolegga Hi, thanks for the quick response! That makes sense from implementation perspective. However, leaving the argument as mandatory is not ideal, imho. As a user, I'd still need to type in this boilerplate argument, which is tedious.

I understand that logger methods can't differentiate between anything we pass in as a context (2nd arg) and first "printf" argument supplied, so in the current setup, there's not much we can do without breaking backward compatibility or introducing a new class/methods.

To be honest, for me it makes little sense to have a "printf" type of string and its arguments divided by a third argument, in the first place.

I don't know what is usual practice, maybe different libraries approached this in some interesting way. I can check out more tomorrow.

[iamolegga]

I totally agree with you. So I can see here two options:

1. Drop context argument, and automatically drop LoggerService compatibility, but technically if you call pino with msg without interpolation tokens and with second argument as string, it will just concat them. So then logger methods will be fully compatible with pino API. And if use logger with this api as NestJS app logger, it will place context argument to msg property for app logs on the start. But I think it's not such a big deal. And also I think that person that is familiar with pino will be expect from pino-logger to be compatible with pino API.
2. Leave current implementation and add one more class with described functionality above, but I think, that having 2 almost same classes may cause a mess.

And frankly, I am more inclined to the first option.

[maricn]

1. Drop context argument, and automatically drop LoggerService compatibility, but technically if you call pino with msg without interpolation tokens and with second argument as string, it will just concat them.

That solves the problem.

So then logger methods will be fully compatible with pino API. And if use logger with this api as NestJS app logger, it will place context argument to msg property for app logs on the start.

But introduces a new one. :)

But I think it's not such a big deal. And also I think that person that is familiar with pino will be expect from pino-logger to be compatible with pino API.

I see this as a choice for who is this package written for: (a) NestJS devs who want to seamlessly use pino logger or (b) pino logger users who are writing a project in NestJS.

Now, I doubt anyone chooses their framework based on logger they're going to use, but surely people that are using NestJS want to use some logger in their app, probably with as little change to NestJS logger as possible. Ofc, some people are familiar with pino API from their other projects, so it's your choice who are you prioritizing. I can't judge on this.

So for me, I want to have request tracing in NestJS project without worrying too much about it, and this package solves that neatly. This feature request would help me by reducing unnecessary boilerplate code and I'd be even more happy. I don't mind either solution. My preference would go for 1. too but only because that fits my needs, not the greater mass of potential users. And that will require bumping up major version, right? EDIT: just saw this is still in beta, so it should be alright to change the API

Perhaps a third one would be to maintain two separate packages. This way 2 almost the same classes wouldn't confuse users, but would still have same (actually bigger) maintenance costs.

[iamolegga]

Yep, the main question here is positioning, but maybe NestJS users will like to set context once per file, actually It is more convenient, less code. So if choose first option, there can be one more feature:

If Logger methods are called in req/res context, then it will be called as pino methods, if call is caused by app itself out of req/res context, like on the start, it will work like now, so there will be expected context property in both cases.

I think this is how it should be for now, what do you think?

[ghost]

I would jump in and agree that my preference is also 1 with the same needs as @maricn .
If can be handled automatically under the hood, the switch between Pino and LoggerService syntax would be great, but I would be also keen to have a fully Pino module and the LoggerService compatible one so that I can pick whichever is better for my needs.

[maricn]

If Logger methods are called in req/res context, then it will be called as pino methods, if call is caused by app itself out of req/res context, like on the start, it will work like now, so there will be expected context property in both cases.

If call is caused by app itself and we handle it with (msg, context, ...args) we might still face the same problem that made me originally open this ticket. Imagine you need to implement any lifecycle event listener. You'd still need type in context in every line, if i got you right?

But, your concern still holds for handling internal NestJS logs by properly implementing LoggerService. That leans me more towards solution 2.

Is it possible to create LoggerService implementation and another class that would internally use LoggerService one and just act as a syntaxic sugar coating (by remembering context and adapting method signatures)? That way we do expose two classes (LoggerService and "convenience" pino logger) but the maintenance costs are only minimally increased.

[iamolegga]

All right, so then there should be two classes:

• class PinoLogger {} with pino core API methods for pino users, how want the same core API and set context one time at injecting
• class Logger implements LoggerService {} for NestJS users, who used built-in Logger, and want the same API

So, I will try to do this in next few days, stay in touch 🙂

[iamolegga]

Hey, I've almost done with it, but just need add tests and docs, so I guess it will be shipped in several hours. The api of new PinoLogger will be like:

import { Controller, Get } from "@nestjs/common";
import { MyService } from "./my.service";
import { PinoLogger } from "../src/PinoLogger";

@Controller()
export class AppController {
  constructor(
    private readonly myService: MyService,
    @PinoLogger.Inject("MY_CONTEXT_VALUE") private readonly logger: PinoLogger
  ) {}

  @Get()
  getHello(): string {
    this.logger.info("getHello(%O)", arguments); // <-- "info" and  "trace" methods (like pino has) as opposite to "log" and "verbose" (like LoggerService has)
    return `Hello ${this.myService.getWorld()}`;
  }
}

And I'm still in doubt, which varian of injection and setting context is better:

1. via @PinoLogger.Inject like above or
2. via normal injection and manually setting context:

import { Controller, Get } from "@nestjs/common";
import { MyService } from "./my.service";
import { PinoLogger } from "../src/PinoLogger";

@Controller()
export class AppController {
  constructor(
    private readonly myService: MyService,
    private readonly logger: PinoLogger
  ) {
    logger.context = "MY_CONTEXT_VALUE";
  }

  @Get()
  getHello(): string {
    this.logger.info("getHello(%O)", arguments); // <-- "info" and  "trace" methods (like pino has) as opposite to "log" and "verbose" (like LoggerService has)
    return `Hello ${this.myService.getWorld()}`;
  }
}

@maricn @valerio-battaglia what do you think?

[maricn]

Looking nice.. :)
I would prefer the first one, so the whole thing is "required" to be setup with decorator in a single step. In the second example, we need a two step process - inject and setup.

One suggestion though, since we're talking about NestJS, everyone using it will often use Inject from @nestjs/common package, and @PinoLogger.Inject might be too expressive. What do you think about renaming your Inject decorator to InjectLogger?

[trymoto]

IMO the second option looks more "natural" and clean, even if i have to manually setup the logger context.

This whole @PinoLogger.Inject("MY_CONTEXT_VALUE") may confuse people (myself included) about its purpose and whether it has anything to do with Nest's DI.

All and all, to me it's just more obvious what i'm supposed to do in this second example.

[iamolegga]

I would agree with @trymoto because I would prefer to KISS, and actually get rid of all not necessary extra API, and make library users not to remember complex API with this @PinoLogger.Inject or InjectLogger.

I think that injecting it as regular service is more preferred, and makes setting context optional, so PinoLogger users may use it without required context setting, and use it just like pino and not thinking about context property, that actually came up from LoggerService 🤔

[maricn]

I see your point now. Thanks for sharing your POV. It makes sense, and you bought me :) Just as another note, maybe it makes sense to use a setter instead of public field?

Just for reference, I'm already using other packages that are handling it with first option. And ofc, the decorator parameter doesn't have to be required.

[iamolegga]

I see your point now. Thanks for sharing your POV. It makes sense, and you bought me :)

Thanks!)

Just as another note, maybe it makes sense to use a setter instead of public field?

It has no side effects for now, and it seems simpler, no?

Just for reference, I'm already using other packages that are handling it with first option.

All right, actually I can try to make both variants, maybe it will be the best choice. Give me just a little time)

[maricn]

Just as another note, maybe it makes sense to use a setter instead of public field?

It has no side effects for now, and it seems simpler, no?

It's your call. I have a different background, and for me personally I'm skeptical of consequences of changing a field, so I usually feel inclined to dig in to verify it. And find it more expressive using API via methods. But that might be totally subjective.

Just for reference, I'm already using other packages that are handling it with first option.

All right, actually I can try to make both variants, maybe it will be the best choice. Give me just a little time)

I'm fine with the first one 👍 , and I think we don't need two options for that. If you start supporting so many different (interchangeable) usage scenarios, you'll just make more stuff to maintain.

[iamolegga]

It's your call. I have a different background, and for me personally I'm skeptical of consequences of changing a field, so I usually feel inclined to dig in to verify it. And find it more expressive using API via methods. But that might be totally subjective.

got you

I'm fine with the first one 👍 , and I think we don't need two options for that. If you start supporting so many different (interchangeable) usage scenarios, you'll just make more stuff to maintain.

That's ok, I already tried it, everything works. So tomorrow I'll finish this

[iamolegga]

closed at v0.5.0