feat(logger): add logger option to all micro components (override DefaultLogger) closes #2556

[asynxc]

closes #2556

(to get a better understanding of the reason bedind this PR, please checkout this issue #2556

A PR to allow providing a custom logger to every micro component (client, server, store, broker,...).

the guide lines of this implementation are :

• do not change components API (for retro compatibility)
• add Logger option to every component
• fallback to DefaultLogger if no logger option provided (for retro compatibility)

Any feedback is welcome ^^

[Davincible]

@asynxc Really nice PR, thanks!

Two thoughts;

In the log helper, you implemented the checks to see if a level is enabled, I know this was present in the original code, but this should be the responsibility of the logger implementation (plugin), so I think we can remove those checks.

Also, if I set the logger for the service globally, I also expect it to be set for all sub-modules/plugins unless specifically specified otherwise. I don't think this is currently implemented right?

[asynxc]

In the log helper, you implemented the checks to see if a level is enabled, I know this was present in the original code, but this should be the responsibility of the logger implementation (plugin), so I think we can remove those checks.

I agree with you, the original code does unnecessarily a double log level check (in the logger and in inline)
Also the logger.Helper does the log level check, but i dit not want to change its behavior/api since it's a public API...

the default logger default and plugins logger does check log level, so i guess i can ditch using the Helper. i have added a commit to do that, please take a look. @Davincible

Also, if I set the logger for the service globally, I also expect it to be set for all sub-modules/plugins unless specifically specified otherwise. I don't think this is currently implemented right?

no it's not the case at the moment, maybe can be done in a second iteration ^^
Edit: Service has no direct access to most components, so in order to pass in the service Logger, components must forward it in to "child" components, and must be careful doing that to not override already setup"child" option logger... so for those reasons i don't think auto setup logger for all components using Service logger is good idea...
The user can always customize Service/global level logger by changing the DefaultLogger
@Davincible thoughts ?

[Davincible]

About the setup of the logger; the problem is that components can be initialized at different stages, which makes it difficult to reliably pass down the logger to all components, unfortunately.

The downside of this is that you might need to define it multiple times.

I'm afraid that passing in a logger will raise the expectation that it will be used everywhere, where this will only be used in the service.

micro.NewService(
    micro.WithLogger(logger)
)

[asynxc]

I'm afraid that passing in a logger will raise the expectation that it will be used everywhere, where this will only be used in the service.

I agree with you, it's a valid assumption.
to accommodate this we can auto set DefaultLogger by the service logger set by the user via micro.WithLogger ?
this way the service logger will used everywhere...

@Davincible thoughts ?

[Davincible]

The problem with just setting the global logger in the option is that there is no guarantee of order. The option can be executed before or after defaults are initialized. The only solution I see, is to make the logger in all services pointers, and by default point to the global variable, which we can then change at any point in time in the setup phase. If a user passes in a custom logger, then the logger wont be pointing to the global anymore

[Davincible]

Turns out that's not how it works. I've been trying to implement it but can't come up with any consistent way. So for now the user will still have to manually set the DefaultLogger, at least you can now manually specify the loggers for the different interfaces

[Davincible]

@asynxc this is ready to be merged now. Have you been able to work on the plugins PR?

[asynxc]

@asynxc this is ready to be merged now. Have you been able to work on the plugins PR?

that's great !

I'm waiting for this PR to land and a go-micro release to be able to reuse the interfaces in plugins

[Davincible]

@asynxc released!

[Davincible]

I'm waiting for this PR to land and a go-micro release to be able to reuse the interfaces in plugins

@asynxc Any progress?