spirit-labs / tektite

Tektite DB
http://www.tektitedb.com
Other
167 stars 25 forks source link

MINOR: Add configurable logger per package #251

Closed stanislavkozlovski closed 5 days ago

stanislavkozlovski commented 1 week ago

This patch extends log.go to provide prefix-logging functionality, allowing each file/struct/component to define their own logger that'll get prepended to every log message:

    log, _ := logger.GetLogger("clustered_data.go")
    log.Debug("Something happened!")
    => "[clustered_data.go] Something happened!"

I hacked around with zap's own benchmark suite and ran the suite 3 times comparing against the usual Zap Sugar logger and this Tektite wrapper. Long-story short:

| Benchmark                               | ns/op      |
|-----------------------------------------|------------|
| Zap.Sugar                               | 58.57      |
| Zap.Sugar_with_TektiteLogger            | 114.00     |
| Zap.SugarFormatting                     | 1275.67    |
| Zap.SugarFormatting_with_TektiteLogger  | 1338.00    |
stanislavkozlovski commented 1 week ago

I also played around with some other ways to write the message - e.g as per this SO suggestion

image

But it wouldn't work, and when I added a lock - it was 2x slower as well, with more complexity. I figured it's not worth it.

stanislavkozlovski commented 1 week ago

image oops... some tests fail

stanislavkozlovski commented 1 week ago

ha ok, I found why some tests were failing. I had removed the

func init() {
    initialise(zapcore.InfoLevel, "console", false)
}

method because I thought it was unused. Learning Go as I go...

purplefox commented 1 week ago

Looks good!

One of the tests is failing: TestTektiteLoggerReturnsErrorIfGlobalLoggerUninitialized- this is because the logger is initialised in a package init() method. I think you will need to move this test to another package without the init() to get this to work.

stanislavkozlovski commented 1 week ago

From some research, it seems like I can't avoid calling the init() method. I put the test elsewhere too and still the same failure.

So I guess this scenario can never happen - which is good. Now I get why you had the override flag