Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEATURE] allow passing custom logger to "server" component #2556

Closed
asynxc opened this issue Sep 22, 2022 · 9 comments · Fixed by #2559
Closed

[FEATURE] allow passing custom logger to "server" component #2556

asynxc opened this issue Sep 22, 2022 · 9 comments · Fixed by #2559

Comments

@asynxc
Copy link
Contributor

asynxc commented Sep 22, 2022

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

in a context where we instantiate multiple server in same process, we may need to assign specific fields to each instance.
So that it's easier to distinguish their logs.

Describe the solution you'd like

add new option server.WithLogger(logger log.Logger) to server options

If this's ok with you, i can create a PR in both go-micro and plugins projects ?

@Davincible
Copy link
Contributor

Davincible commented Sep 22, 2022

Thanks for the feature request, PRs are definitely most welcome.

In this case, however, the service uses go-micro.dev/v4/logger.DefaultLogger as a logger, which is the same for the entire runtime. To use a different logger per service quite a bit would need to be changed.

The easy solution here would be to separate your services out in one per runtime.

Edit: you can ofcourse specify the logger by manually overriding that value

@asynxc
Copy link
Contributor Author

asynxc commented Sep 23, 2022

Thanks for your feedback

The easy solution here would be to separate your services out in one per runtime.

The thing is about logger, is that we need to customize them even in the same runtime/microservice, usage example, when filtering logs of the broker client component, i may need to use a specific field (ex: broker) or for the store component or any other component use in microservice.

To use a different logger per service quite a bit would need to be changed.

i have read the codebase, DefaultLogger seems to be used everywere. but i'm still willing to create a PR that provide the option to override that and fallback to DefaultLogger in case no option logger provided, so that it stay backward compatible.

What do you think ?

@Davincible
Copy link
Contributor

Davincible commented Sep 23, 2022

Your use case makes sense.

Definitely open to better implementations.

Well written PR is welcome. Looking forward to see your implementation :)

@xpunch thoughs on this ?

Edit: one note here, is that to use custom loggers per interface (store, registry, broker etc.) All plugins need to be rewritten alongside of the new options to use them. Perhaps we can by default auto derive an extra field to denote the interface type as you suggested

@asynxc
Copy link
Contributor Author

asynxc commented Sep 23, 2022

@Davincible the fallback to DefaultLogger if no logger option provided allow us to handle plugins afterword without breaking anything...

in order for me to introduce this feature, i'll create two PRs, one for go-micro, second for plugins after go-micro release.

@xpunch
Copy link
Contributor

xpunch commented Sep 26, 2022

The logger can be abstracted as a configurable component like registry, store, broker etc.
I think it can have a default implementation, and user can also have its own implementations, and replace it by import custom plugins.
https://github.com/go-micro/go-micro/blob/master/util/cmd/cmd.go#L49

@jochumdev
Copy link
Contributor

Still there's a global containing the Logger for the current runtime? I believe @asynxc want's to replace the global with a Logger per instance.

@Davincible
Copy link
Contributor

@pcdummy global logger won't be removed, see the PR for details. It's only used as a fall back

@Davincible
Copy link
Contributor

@xpunch if you make the logger only one configurable component it's still global, just not in a global variable.

By allowing to pass in loggers through options you can set a different logger for different components (broker, registry etc.), so you can add custom fields to each logger, which what asynxc is looking for

@xpunch
Copy link
Contributor

xpunch commented Sep 27, 2022

@Davincible I got your point, logging is very important in micro service systems. Different component or plugins may need different logger, maybe we need to re-design the whole logging modules in go micro.

Davincible added a commit that referenced this issue Sep 29, 2022
…aultLogger) closes #2556 (#2559)

* feat(logger): add logger option to all components

* fix: refactor api/rpc.go

* fix: refactor api/stream.go

* fix: api/options.go comment

* fix(logger): do not use logger.Helper internally

* fix(logger): fix comments

* fix(logger): use level.Enabled method

* fix: rename mlogger to log

* fix: run go fmt

* fix: log level

* fix: factories

Co-authored-by: Mohamed MHAMDI <mmhamdi@hubside.com>
Co-authored-by: Davincible <david.brouwer.99@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants