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

issues with logging library #4055

Closed
Dieterbe opened this issue Feb 17, 2016 · 3 comments
Closed

issues with logging library #4055

Dieterbe opened this issue Feb 17, 2016 · 3 comments

Comments

@Dieterbe
Copy link
Contributor

Here's some things i noticed about the logging library

  • what's with the adapter registration stuff? why is there a need for the "adapters" datastructure? instead of having people do like log.NewLogger(0, "console", config) you could just do log.NewLogger, 0, log.NewConsole(config)). the compiler is a pretty good system for assuring you don't have two identically named functions and that the code doesn't call functions which don't exist. So all this checking for proper adapter strings, "called register twice" checks, is not needed if people can just call the constructors directly.
  • why the separate Init step, why not just pass in the config when creating the object?
  • why does the API use json config data? it's ironic that grafana has to parse the grafana config file,
    then generate a json config, pass that to the log lib, and have the log lib parse the json and return errors at runtime. Using actual parameters or structs would be much more robust (and fast). The compiler can do static compile time checks for your config (from the log lib's API perspective at least), and you can easily reference the actual levels instead of having to specify numbers.
  • creating a new logger is very misleading and performs needlessly poorly. I.e. in metric_tank, which uses this lib, we do:
    log.NewLogger(0, "console", fmt.Sprintf(`{"level": %d, "formatting":true}`, *logLevel))

what this actually does, is create a new logger (with no control over settings such as loglevel), and the "console logger" is just an "output" for said auto-created logger. not only does this seem like a needless overcomplication, it will cause needless work. e.g. on Debug() calls some allocation work, then call writerMsg (more allocations and various work), then a channel write and channel read, only then to just send the message to the one output's WriteMsg, only at this point do we hit the thing we configured, the "output" which can decide to discard the message based on the level. needless to say performance features like bd2e9a8 don't work in this scenario. am I doing something wrong? I've been looking for a way to set the level on the actual "logger" then but there doesn't seem to be a facility for this at all.

@torkelo
Copy link
Member

torkelo commented Feb 17, 2016

I have no idea, I have not written the log library :)

I assume the log.NewLogger(0, "console", config) syntax is for supporting configuring loggers via config file. The only place that creates loggers is code in setting.go that read the logging config from the ini file.

Not sure the log lib supports / is built for using separate logger instance per use case, but have not looked at it very closely :(

@torkelo
Copy link
Member

torkelo commented Feb 17, 2016

oh regarding the json config, yea, find that strange as well.

@torkelo
Copy link
Member

torkelo commented Feb 17, 2016

very open to new log lib / PR that improves the built in log lib, but this issue seemed more like questions so closed this. Please let me know if there is any unanswered question

Dieterbe added a commit to raintank/grafana that referenced this issue Feb 17, 2016
performance workaround, see grafana#4055
Dieterbe added a commit to grafana/metrictank that referenced this issue Feb 17, 2016
Dieterbe added a commit that referenced this issue Feb 18, 2016
performance workaround, see #4055
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants