Switch logging to go-kit #144

sowmiyamuthuraman · 2019-10-01T02:24:22Z

Fixes #141

brian-brazil · 2019-10-01T09:25:32Z

Code looks good, can you add the DCO? Test failure looks like a CI issue, retrying.

brian-brazil · 2019-10-01T09:25:55Z

Ah, you need to update the vendoring.

sowmiyamuthuraman · 2019-10-01T17:56:41Z

@brian-brazil Thanks for the input! updated the vendoring and ammended to the same commit.

simonpasquier · 2019-10-02T09:14:23Z

@sowmiyamuthuraman you need to update scripts/errcheck_excludes.txt to pass the CI (see here).

simonpasquier · 2019-10-15T12:49:00Z

@sowmiyamuthuraman can you rebase on master and update the scripts/errcheck_excludes.txt file?

Signed-off-by: sowmiya <[email protected]>

sowmiyamuthuraman · 2019-10-15T15:14:21Z

@simonpasquier i resolved the conflicts and updated the scripts/errcheck_excludes.txt. Can you please help in passing the test?

Signed-off-by: Simon Pasquier <[email protected]>

simonpasquier · 2019-10-16T07:57:55Z

I've pushed an update to your branch to fix the go.mod file and remove the .idea directory. Make sure you update your local directory.

Tests are failing because Exporter depends on the global logger instance which isn't set in the tests.
You need to add a log.Logger field to the Exporter struct and update NewExporter() accordingly. Then you can pass a log.NewNopLogger() in the tests.

Signed-off-by: sowmiyamuthuraman <[email protected]>

sowmiyamuthuraman · 2019-10-16T13:27:24Z

I've pushed an update to your branch to fix the go.mod file and remove the .idea directory. Make sure you update your local directory.

Tests are failing because Exporter depends on the global logger instance which isn't set in the tests.
You need to add a log.Logger field to the Exporter struct and update NewExporter() accordingly. Then you can pass a log.NewNopLogger() in the tests.

Thanks, @simonpasquier. I have made the suggested changes and amended with the same commit

simonpasquier

Good work! Just a few comments and it should be good to go.

simonpasquier · 2019-10-17T12:07:33Z

consul_exporter.go

@@ -170,6 +177,7 @@ func NewExporter(opts consulOpts, kvPrefix, kvFilter string, healthSummary bool)
 		kvPrefix:      kvPrefix,
 		kvFilter:      regexp.MustCompile(kvFilter),
 		healthSummary: healthSummary,
+		logger:        log.NewLogfmtLogger(os.Stdout),


You should modify the NewExporter function to add a log.Logger parameter and pass it here.

simonpasquier · 2019-10-17T12:10:34Z

consul_exporter.go


 func (l promHTTPLogger) Println(v ...interface{}) {
-	log.Error(v...)
+	level.Error(l.logger).Log("msg", v)


rather level.Error(l.logger).Log("msg", fmt.Sprintf(v))

simonpasquier · 2019-10-17T12:10:59Z

consul_exporter.go

@@ -441,7 +452,9 @@ func main() {
 			promhttp.HandlerFor(
 				prometheus.DefaultGatherer,
 				promhttp.HandlerOpts{
-					ErrorLog: &promHTTPLogger{},
+					ErrorLog: &promHTTPLogger{
+						logger: log.NewLogfmtLogger(os.Stderr),


logger instead of log.NewLogfmtLogger(os.Stderr)

simonpasquier · 2019-10-17T12:11:31Z

consul_exporter.go


 	exporter, err := NewExporter(opts, *kvPrefix, *kvFilter, *healthSummary)
 	if err != nil {
-		log.Fatalln(err)
+		level.Error(logger).Log("msg", "Error creating an exporter", "err", err)


(nit) Error creating the exporter

simonpasquier · 2019-10-17T12:13:36Z

consul_exporter.go

-	log.Infoln("Starting consul_exporter", version.Info())
-	log.Infoln("Build context", version.BuildContext())
+	level.Info(logger).Log("msg", "Starting consul_exporter", "version", version.Info())
+	level.Info(logger).Log("msg", "Build context", version.BuildContext())


Arguments to Log() should always be even (key+value) so:

level.Info(logger).Log("build_context", version.BuildContext())

simonpasquier · 2019-10-17T12:14:02Z

consul_exporter.go

@@ -330,14 +338,14 @@ func (e *Exporter) collectHealthSummary(ch chan<- prometheus.Metric, serviceName
 func (e *Exporter) collectOneHealthSummary(ch chan<- prometheus.Metric, serviceName string) {
 	// See https://github.com/hashicorp/consul/issues/1096.
 	if strings.HasPrefix(serviceName, "/") {
-		log.Warnf("Skipping service %q because it starts with a slash", serviceName)
+		level.Warn(e.logger).Log("msg", "Skipping service because it starts with a slash", "service name", serviceName)


"service name" -> "service_name"

Signed-off-by: sowmiyamuthuraman <[email protected]>

simonpasquier · 2019-10-18T07:51:08Z

consul_exporter.go


 func (l promHTTPLogger) Println(v ...interface{}) {
-	log.Error(v...)
+	level.Error(l.logger).Log("msg", fmt.Sprintf("%v", v))


Sorry my previous comment was confusing, it should be level.Error(l.logger).Log("msg", fmt.Sprint(v...))

simonpasquier · 2019-10-18T07:55:49Z

As #145 got merged, you'd need to rebase on master and adjust the changes. Sorry for the incovenience!

Signed-off-by: sowmiyamuthuraman <[email protected]>

simonpasquier

👍

simonpasquier · 2019-10-22T12:49:34Z

Thanks for the work!

sowmiyamuthuraman force-pushed the Fix/#141 branch from 07e4f06 to b430f04 Compare October 1, 2019 02:31

sowmiyamuthuraman force-pushed the Fix/#141 branch 2 times, most recently from 43a649e to dd35b0e Compare October 1, 2019 17:51

sowmiyamuthuraman force-pushed the Fix/#141 branch 2 times, most recently from 452dc52 to 07eeca7 Compare October 1, 2019 18:00

sowmiyamuthuraman force-pushed the Fix/#141 branch 4 times, most recently from 2e80952 to 0f1e157 Compare October 2, 2019 15:16

Switch logging to go-kit

48b88be

Signed-off-by: sowmiya <[email protected]>

sowmiyamuthuraman force-pushed the Fix/#141 branch from 0f1e157 to 48b88be Compare October 15, 2019 15:07

Fix go.mod and remove .idea directory

15ea080

Signed-off-by: Simon Pasquier <[email protected]>

Fix initialise logger on creation of the Exporter struct

e1d68a6

Signed-off-by: sowmiyamuthuraman <[email protected]>

sowmiyamuthuraman force-pushed the Fix/#141 branch from 16d7bd5 to e1d68a6 Compare October 16, 2019 13:12

simonpasquier reviewed Oct 17, 2019

View reviewed changes

Pass logger as params to the new exporter function

0b54ba7

Signed-off-by: sowmiyamuthuraman <[email protected]>

simonpasquier reviewed Oct 18, 2019

View reviewed changes

sowmiyamuthuraman force-pushed the Fix/#141 branch from 1ae411c to 7993c6b Compare October 19, 2019 07:46

Fix merge conflicts

b9e4941

Signed-off-by: sowmiyamuthuraman <[email protected]>

sowmiyamuthuraman force-pushed the Fix/#141 branch from 7993c6b to b9e4941 Compare October 19, 2019 07:52

simonpasquier approved these changes Oct 22, 2019

View reviewed changes

simonpasquier merged commit b68117b into prometheus:master Oct 22, 2019

simonpasquier added the change label Dec 11, 2019

djrodgerspryor mentioned this pull request Mar 29, 2023

Missing syslog support after go-kit upgrade #266

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch logging to go-kit #144

Switch logging to go-kit #144

sowmiyamuthuraman commented Oct 1, 2019

brian-brazil commented Oct 1, 2019

brian-brazil commented Oct 1, 2019

sowmiyamuthuraman commented Oct 1, 2019

simonpasquier commented Oct 2, 2019

simonpasquier commented Oct 15, 2019

sowmiyamuthuraman commented Oct 15, 2019

simonpasquier commented Oct 16, 2019

sowmiyamuthuraman commented Oct 16, 2019

simonpasquier left a comment

simonpasquier Oct 17, 2019

simonpasquier Oct 17, 2019

simonpasquier Oct 17, 2019

simonpasquier Oct 17, 2019

simonpasquier Oct 17, 2019

simonpasquier Oct 17, 2019

simonpasquier Oct 18, 2019 •

edited

Loading

simonpasquier commented Oct 18, 2019

simonpasquier left a comment

simonpasquier commented Oct 22, 2019

Switch logging to go-kit #144

Switch logging to go-kit #144

Conversation

sowmiyamuthuraman commented Oct 1, 2019

brian-brazil commented Oct 1, 2019

brian-brazil commented Oct 1, 2019

sowmiyamuthuraman commented Oct 1, 2019

simonpasquier commented Oct 2, 2019

simonpasquier commented Oct 15, 2019

sowmiyamuthuraman commented Oct 15, 2019

simonpasquier commented Oct 16, 2019

sowmiyamuthuraman commented Oct 16, 2019

simonpasquier left a comment

Choose a reason for hiding this comment

simonpasquier Oct 17, 2019

Choose a reason for hiding this comment

simonpasquier Oct 17, 2019

Choose a reason for hiding this comment

simonpasquier Oct 17, 2019

Choose a reason for hiding this comment

simonpasquier Oct 17, 2019

Choose a reason for hiding this comment

simonpasquier Oct 17, 2019

Choose a reason for hiding this comment

simonpasquier Oct 17, 2019

Choose a reason for hiding this comment

simonpasquier Oct 18, 2019 • edited Loading

Choose a reason for hiding this comment

simonpasquier commented Oct 18, 2019

simonpasquier left a comment

Choose a reason for hiding this comment

simonpasquier commented Oct 22, 2019

simonpasquier Oct 18, 2019 •

edited

Loading