Feature: Add log api #144

andig · 2020-07-04T11:43:48Z

expose public Logger api
implement a default console logger
add an internal LogAdapter that connected client with public logger
make SetDebugLevel part of LogAdapter

Briefly describe your proposed changes:

CHANGELOG.md updated
Rebased/mergeable
Tests pass
Sign CLA (if not already signed)

codecov-commenter · 2020-07-04T11:46:06Z

Codecov Report

Merging #144 into master will decrease coverage by 0.17%.
The diff coverage is 63.41%.

@@            Coverage Diff             @@
##           master     #144      +/-   ##
==========================================
- Coverage   78.75%   78.57%   -0.18%     
==========================================
  Files          26       27       +1     
  Lines        1784     1802      +18     
==========================================
+ Hits         1405     1416      +11     
- Misses        252      259       +7     
  Partials      127      127

Impacted Files	Coverage Δ
api/write.go	`94.11% <ø> (ø)`
client.go	`84.15% <ø> (ø)`
api/log/logger.go	`62.50% <62.50%> (ø)`
internal/log/logger.go	`65.38% <64.00%> (-1.29%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e21926f...a221301. Read the comment docs.

vlastahajek · 2020-07-08T14:51:29Z

@andig, thanks for the PR!
The current logging mechanism and API will be examined, so stay tuned.

sranka

a naming nit: there are now two directories that contain log implementation "/api/log" and "/internal/log" ... these are tightly connected to each other

/api/log is (unlike others in /api) not used for influxDB API, but for API for the client API
/internal/log might not be considered internal anymore

... refactoring to a new simple /log directly might look better

andig · 2020-07-16T08:07:07Z

these are tightly connected to each other

api/log currently (and historically) contains the internal-only logger implementation- I'd like to move that to /internal/log
package needs to include /internal/log to use log.Log. It is easy to mistake this with api/log- let's move api/log to api, containing only the interface (and the instance) to avoid mistakes
last we might consider if we should move the Log instance to a client option. This would be a bit more invasive.

Makes sense?

vlastahajek · 2020-07-16T08:51:50Z

I plan to use Uber zap.Logger as a default Logger. As it has not its own interface, it still makes sense to add a logger interface. The best place is to put it to the /log folder, as Pavel suggested. /api folder is intended for the server REST API interfaces and implementations.
I still don't have a clear idea of how it should finally look like, I will get to this at the beginning of next week.
There will be regularly scheduled release with mostly bug fixes tomorrow, so the logging improvement will be, for sure now, in the release after.

vlastahajek · 2020-07-21T12:40:09Z

api/log/logger.go

+// Use of this source code is governed by MIT
+// license that can be found in the LICENSE file.
+
+package log


Move it to the /logger, please.

vlastahajek · 2020-07-21T12:52:56Z

api/log/logger.go

+type logLogger struct{}
+
+func (l *logLogger) Debugf(format string, v ...interface{}) {
+	log.Print("[D]! ", fmt.Sprintf(format, v...))
+}
+
+func (l *logLogger) Debug(msg string) {
+	log.Print("[D]! ", msg)
+}
+
+func (l *logLogger) Infof(format string, v ...interface{}) {
+	log.Print("[I]! ", fmt.Sprintf(format, v...))
+}
+
+func (l *logLogger) Info(msg string) {
+	log.Print("[I]! ", msg)
+}
+
+func (l *logLogger) Warnf(format string, v ...interface{}) {
+	log.Print("[W]! ", fmt.Sprintf(format, v...))
+}
+
+func (l *logLogger) Warn(msg string) {
+	log.Print("[W]! ", msg)
+}
+
+func (l *logLogger) Errorf(format string, v ...interface{}) {
+	log.Print("[E]! ", fmt.Sprintf(format, v...))
+}
+
+func (l *logLogger) Error(msg string) {
+	log.Print("[E]! ", msg)
+}


I think I undestand your idea behind having logLogger and LogAdapater, but it seems too much for just a simple logging mechanism.
Just keep the original implementation as the default implementation.
I would also create LogConfig structure that will hold logging configuration. There is the LogLevel property and I will add also Prefix to prefix all logged lines. This will configure the logger via SetConfig func.

vlastahajek · 2020-07-22T07:43:04Z

internal/log/logger.go

+var Log LogAdapter

-// Logger provides filtered and categorized logging API.
-// It logs to standard logger, only errors by default
-type Logger struct {
+// LogAdapter provides filtered and categorized logging API.
+// It logs using the api logger instance. Only errors are logged by default.
+type LogAdapter struct {
 	debugLevel uint
 }

 // SetDebugLevel to filter log messages. Each level mean to log all categories bellow
 // 0 errors , 1 - warning, 2 - info, 3 - debug
-func (l *Logger) SetDebugLevel(debugLevel uint) {
+
+func (l *LogAdapter) SetDebugLevel(debugLevel uint) {
 	l.debugLevel = debugLevel
 }

-func (l *Logger) Debugf(format string, v ...interface{}) {
-	if l.debugLevel > 2 {
-		log.Print("[D]! ", fmt.Sprintf(format, v...))
+func (l *LogAdapter) Debugf(format string, v ...interface{}) {
+	if log.Log != nil && l.debugLevel > 2 {
+		log.Log.Debugf(format, v...)
 	}
 }
-func (l *Logger) Debug(msg string) {
-	if l.debugLevel > 2 {
-		log.Print("[D]! ", msg)
+
+func (l *LogAdapter) Debug(msg string) {
+	if log.Log != nil && l.debugLevel > 2 {
+		log.Log.Debug(msg)
 	}
 }

-func (l *Logger) Infof(format string, v ...interface{}) {
-	if l.debugLevel > 1 {
-		log.Print("[I]! ", fmt.Sprintf(format, v...))
+func (l *LogAdapter) Infof(format string, v ...interface{}) {
+	if log.Log != nil && l.debugLevel > 1 {
+		log.Log.Infof(format, v...)
 	}
 }
-func (l *Logger) Info(msg string) {
-	if l.debugLevel > 1 {
-		log.Print("[I]! ", msg)
+
+func (l *LogAdapter) Info(msg string) {
+	if log.Log != nil && l.debugLevel > 1 {
+		log.Log.Info(msg)
 	}
 }

-func (l *Logger) Warnf(format string, v ...interface{}) {
-	if l.debugLevel > 0 {
-		log.Print("[W]! ", fmt.Sprintf(format, v...))
+func (l *LogAdapter) Warnf(format string, v ...interface{}) {
+	if log.Log != nil && l.debugLevel > 0 {
+		log.Log.Warnf(format, v...)
 	}
 }
-func (l *Logger) Warn(msg string) {
-	if l.debugLevel > 0 {
-		log.Print("[W]! ", msg)
+
+func (l *LogAdapter) Warn(msg string) {
+	if log.Log != nil && l.debugLevel > 0 {
+		log.Log.Warn(msg)
 	}
 }

-func (l *Logger) Errorf(format string, v ...interface{}) {
-	log.Print("[E]! ", fmt.Sprintf(format, v...))
+func (l *LogAdapter) Errorf(format string, v ...interface{}) {
+	if log.Log != nil {
+		log.Log.Errorf(format, v...)
+	}
 }

-func (l *Logger) Error(msg string) {
-	log.Print("[E]! ", msg)
+func (l *LogAdapter) Error(msg string) {
+	if log.Log != nil {
+		log.Log.Error(msg)
+	}
 }


There should be the default implementation with the original code. init() could initially set logger.Log

andig · 2020-07-22T10:17:15Z

@vlastahajek appreciate your comments but not sure I fully follow. I was trying to achieve 3 4 goals:

have a public logger interface
separate setting the debug level from the logger interface (it should really be part of client options, but thats another topic)
keep the default implementation (using console logger)
clean the implementation's interface such that the [E] prefixes become part of the default implementation, not of the logger interface

If that makes sense I could take another stab at it

vlastahajek · 2020-07-22T13:24:41Z

@andig, my comments mainly target the fact your proposal introduces two logging objects, the default implementation, logger, and LogAdapter.

In fact, in the actual state, the internal/log/Logger is already a log adapter, because it uses the standard log package under the hood. If you need to override the log target, which is essentially what Logger interface in your design means, you could just replace the actual log writer in the standard log package, without the necessity to have a logger interface.

As the filtering capability, and also prefixing, is part of the logger responsibilities, the logger should handle that directly.
SetLogLevel of Client options will simply forward this to the current logger.

andig · 2020-07-22T14:55:16Z

As the filtering capability, and also prefixing, is part of the logger responsibilities, the logger should handle that directly.
SetLogLevel of Client options will simply forward this to the current logger.

Prefixing: ack

Filtering: I think it comes down to where you want the SetLogLevel handled. Imho that is part of the client duties- it should only log what it is supposed to log. With that approach the adapter is needed or a lot if if/elses across the codebase. Same if you want to be able to nil the logger. I'd propose to keep the log interface slim and handle SetLogLevel in this library.

vlastahajek · 2020-07-27T13:05:59Z

As Logger defines what levels are supported, Logger should be responsible for filtering of those levels.
This is how third-party loggers behave.

vlastahajek · 2020-08-13T15:30:29Z

@andig, thanks for your effort. I had to complete this, so I implemented that according to my thoughts. I hope it still will be useful for your needs.

andig · 2020-08-13T15:38:30Z

Thank you- just didn‘t get around to actually finish this, sorry.

Add log api

6f8f98f

andig changed the title ~~Add log api~~ Feature: Add log api Jul 4, 2020

Update changelog

e0c4fc4

andig mentioned this pull request Jul 4, 2020

Make logger configurable #104

Closed

andig force-pushed the logger-api branch 3 times, most recently from c17067d to eb2a27c Compare July 4, 2020 12:07

Remove SetDebugLevel from logger api

a221301

andig force-pushed the logger-api branch from eb2a27c to a221301 Compare July 4, 2020 12:13

russorat requested a review from GeorgeMac July 6, 2020 18:38

sranka reviewed Jul 16, 2020

View reviewed changes

vlastahajek requested changes Jul 22, 2020

View reviewed changes

vlastahajek reviewed Jul 22, 2020

View reviewed changes

vlastahajek mentioned this pull request Aug 13, 2020

feat: Adding public logging API #180

Merged

5 tasks

vlastahajek closed this in #180 Aug 13, 2020

andig deleted the logger-api branch August 13, 2020 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Add log api #144

Feature: Add log api #144

andig commented Jul 4, 2020 •

edited

codecov-commenter commented Jul 4, 2020 •

edited

vlastahajek commented Jul 8, 2020

sranka left a comment

andig commented Jul 16, 2020 •

edited

vlastahajek commented Jul 16, 2020

vlastahajek Jul 21, 2020

vlastahajek Jul 21, 2020

vlastahajek Jul 22, 2020

andig commented Jul 22, 2020 •

edited

vlastahajek commented Jul 22, 2020

andig commented Jul 22, 2020

vlastahajek commented Jul 27, 2020

vlastahajek commented Aug 13, 2020 •

edited

andig commented Aug 13, 2020

Feature: Add log api #144

Feature: Add log api #144

Conversation

andig commented Jul 4, 2020 • edited

codecov-commenter commented Jul 4, 2020 • edited

Codecov Report

vlastahajek commented Jul 8, 2020

sranka left a comment

Choose a reason for hiding this comment

andig commented Jul 16, 2020 • edited

vlastahajek commented Jul 16, 2020

vlastahajek Jul 21, 2020

Choose a reason for hiding this comment

vlastahajek Jul 21, 2020

Choose a reason for hiding this comment

vlastahajek Jul 22, 2020

Choose a reason for hiding this comment

andig commented Jul 22, 2020 • edited

vlastahajek commented Jul 22, 2020

andig commented Jul 22, 2020

vlastahajek commented Jul 27, 2020

vlastahajek commented Aug 13, 2020 • edited

andig commented Aug 13, 2020

andig commented Jul 4, 2020 •

edited

codecov-commenter commented Jul 4, 2020 •

edited

andig commented Jul 16, 2020 •

edited

andig commented Jul 22, 2020 •

edited

vlastahajek commented Aug 13, 2020 •

edited