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

Initialize logger with prom config from flags #13

Conversation

brennantaylor
Copy link
Contributor

It looks like the logger is initialized with an empty config, then a different
config is passed into kingpin to be filled up from the command line arguments.

This change re-initializes the logger with the prom config parsed by kingpin so
that --log.level and --log.format work as documented.

@n-oden
Copy link
Contributor

n-oden commented Jan 15, 2022

@brennantaylor thank you so much; this one was driving me somewhat nuts and I couldn't figure out where the breakage was.

@pengbo0328
Copy link
Collaborator

pengbo0328 commented Jan 17, 2022

@brennantaylor
Thank you!
It seems your patch doesn't fix this issue. Did I miss any configurations?

./pgpool2_exporter --log.level=error
ts=2022-01-17T08:06:13.818Z caller=log.go:168 level=debug msg="Querying namespace" namespace=pool_processes
ts=2022-01-17T08:06:13.821Z caller=log.go:168 level=debug msg="Querying namespace" namespace=pool_cache
ts=2022-01-17T08:06:13.821Z caller=log.go:168 level=debug msg="Querying namespace" namespace=pool_nodes
ts=2022-01-17T08:06:13.842Z caller=log.go:168 level=debug msg="Querying Pgpool-II version"
ts=2022-01-17T08:06:13.844Z caller=log.go:168 level=debug pgpool_version=4.3.0
ts=2022-01-17T08:06:13.844Z caller=log.go:168 level=info msg="Starting pgpool2_exporter" version="(version=, branch=, revision=)" dsn="postgresql://pengbo:MASKED_PASSWORD@127.0.0.1:11000/test?sslmode=disable"
ts=2022-01-17T08:06:13.844Z caller=log.go:168 level=info msg="Listening on address" address=:9719

@brennantaylor
Copy link
Contributor Author

Interesting I was just testing json. I'll take a peek at the log levels.

Without my patch:

dbox:~/ghq/github.com/brennantaylor/pgpool2_exporter$ go run . --log.format=json
ts=2022-01-18T04:16:10.766Z caller=log.go:168 level=error err="error connecting to Pgpool-II: dial tcp [::1]:5432: connect: connection refused"

With my patch json formatting turns on.

 go run . --log.format=json 2>&1 | jq
{
  "caller": "log.go:168",
  "err": "error connecting to Pgpool-II: dial tcp [::1]:5432: connect: connection refused",
  "level": "error",
  "ts": "2022-01-18T04:17:33.769Z"
}

@brennantaylor
Copy link
Contributor Author

I poked around a bit but found nothing obvious. The --log.level param is being read and it sets the field on the config properly. It is then passed into new which creates a NewFilter. But indeed log messages don't seem to respect the level.

@brennantaylor
Copy link
Contributor Author

brennantaylor commented Jan 18, 2022

Oh I think I figured it out.

  • promlog is using the smaller github.com/go-kit/log/level
  • pgpool2_exporter is using github.com/go-kit/kit/log/level

I believe go-kit/log exists in case someone wants to use the logging code without pulling in the rest of go-kit but that means we're cross wiring logger instances.

So just a library mixup. I'll try changing pgpool2_exporter to use the same library promlog is using.

This addresses two problems.

First it looks like the logger is initialized with an empty config, then
a different config is passed into kingpin to be filled up from the
command line arguments.

This change re-initializes the logger with the prom config parsed by
kingpin so that `--log.level` and `--log.format` populate the promlog
instance as expected.

The second issue is that there are 2 go-kit logging libraries.
The smaller `go-kit/log` and `go-kit/kit/log`.

`promlog` is using the smaller `go-kit/log` so `pgpool2_exporter` has
been updated to use the same. Otherwise we were using different
instances of the level types which means the following type assertion
would fail inside the filtered logger since the level is from the other
package:

```go
if v, ok := keyvals[i].(*levelValue); ok {
```
@brennantaylor brennantaylor force-pushed the initialize-logger-with-config-from-flags branch from 33dfcc4 to 1069e32 Compare January 18, 2022 05:15
@brennantaylor
Copy link
Contributor Author

@pengbo0328 I pushed the changes, you should see that both --log.level and --log.format work.

@brennantaylor
Copy link
Contributor Author

@pengbo0328 Just letting you know this is ready for you to take another look at.

@pengbo0328
Copy link
Collaborator

@brennantaylor
Thank you!
I have tested your patch and it fixes the logger problems.

@pengbo0328 pengbo0328 merged commit d30ec6b into pgpool:master Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants