Add support for zap v1.x APIs #11

Vasilis · 2017-04-29T00:26:12Z

Before opening your pull request, please make sure that you have:

signed Uber's Contributor License Agreement;
added tests to cover your changes;
run the test suite locally (make test); and finally,
run the linters locally (make lint).

Thanks for your contribution!

henridevieux · 2017-05-01T14:20:03Z

glide.yaml

@@ -9,10 +9,9 @@ import:
 - package: github.com/spacemonkeygo/monotime
 - package: github.com/uber/uber-licence
 - package: github.com/uber-go/zap


I think its a good idea to pin to ^1 to avoid breaking changes if they release 2.x.x

henridevieux · 2017-05-01T14:27:22Z

internal/log/log.go

 	if c.LogSink != "" {
-		sink, err := os.OpenFile(c.LogSink, os.O_RDWR|os.O_APPEND|os.O_CREATE, 0644)
+		_, err := os.OpenFile(c.LogSink, os.O_RDWR|os.O_APPEND|os.O_CREATE, 0644)


You don't need this code if you add the file path to config.OutputPaths. The config.Build() will then end up calling zap.Open() on the file.

billf · 2017-05-03T00:47:44Z

collector/collector_test.go

@@ -28,12 +28,14 @@ import (

 	"github.com/spacemonkeygo/monotime"
 	"github.com/stretchr/testify/assert"
-	"github.com/uber-go/zap"
 	"github.com/uber/arachne/config"


github.com/uber/arachne are local includes that should be separate from the externals

They should still be separate from standard libs, e.g. "net", "sync" right? Please see next diff.

billf · 2017-05-03T00:49:31Z

internal/tcp/tcp.go

@@ -522,7 +524,8 @@ func Receiver(
 				// Received SYN+ACK (Open port)
 				logger.Debug("Received",
 					zap.String("flag", "SYN ACK"),
-					zap.Object("port", pkt.srcPort))
+					zap.String("src_address", fromAddrStr),
+					zap.Any("src_port", pkt.srcPort))


zap.Uint16 otherwise these use reflection

(comment repeats throughout)

The docs actually over state things:

zap.Any is just a big type assertion https://github.com/uber-go/zap/blob/master/field.go#L233

the final case there uses reflection, but everything else has a much faster path

now, this is micro slow because it needs to allocate an uint16 on the heap to pass it as interface{} (until go 1.9)

so yeah, probably stop using zap.Any in hot paths for sure, but the reason isn't primarily "reflection" in this case

Addressed! Thank you, @billf and @jcorbin

billf · 2017-05-03T00:51:32Z

collector/collector.go

@@ -514,7 +516,8 @@ func printTableEntry(
 		logger.Info("Result",
 			zap.String("host", targetHost),
 			zap.Int("source_port", int(srcPort)),


this is supported natively

henridevieux · 2017-05-03T15:42:04Z

Looks good

jcorbin

Initial skim, will finish read through later.

jcorbin · 2017-05-03T16:39:30Z

bootstrap.go

@@ -41,7 +41,12 @@ func Run(ec *config.Extended, opts ...Option) {
 		err error
 	)

-	bootstrapLogger := zap.New(zap.NewJSONEncoder())
+	bl, _ := zap.NewProduction()


You really should check that error and panic or call core log.Fatal(err) on it; I'd suggest the latter:

import coreLog "log" bl, err := zap.NewProduction() if err != nil { coreLog.Fatal(err) }

Done. Thanks!

jcorbin · 2017-05-03T16:42:00Z

internal/log/log.go

+		bootstrapLogger.Error("Log level provided", zap.Error(err))
+	}
+
+	config := zap.Config{


Why not take this all the way back into your config shape? Compatibility? Any plans to do so in the future?

Done. zap.Config now created in config.Get() and passed to log.CreateLogger()

jcorbin · 2017-05-03T16:43:48Z

internal/log/log.go

-			output),
+	l, err := config.Build()
+	if err != nil {
+		return nil, errors.New("failed to create logger")


don't hide the reason why, at least do a fmt.Errorf("failed to create loger: %v", err)

preferably using github.com/pkg/errors

The err message is not hidden, but displayed by the calling function:

logger, err := log.CreateLogger(&gl.App.Logging, d.ArachneService, osHostname, gl.App.PIDPath, util.RemovePID, *(gl.CLI.Foreground), bootstrapLogger) if err != nil { bootstrapLogger.Error("unable to initialize Arachne Logger", zap.Error(err)) os.Exit(1) }

Now using github.com/pkg/errors globally.

jcorbin · 2017-05-03T16:44:19Z

internal/tcp/tcp.go

@@ -522,7 +524,8 @@ func Receiver(
 				// Received SYN+ACK (Open port)
 				logger.Debug("Received",
 					zap.String("flag", "SYN ACK"),
-					zap.Object("port", pkt.srcPort))
+					zap.String("src_address", fromAddrStr),
+					zap.Any("src_port", pkt.srcPort))


The docs actually over state things:

zap.Any is just a big type assertion https://github.com/uber-go/zap/blob/master/field.go#L233

the final case there uses reflection, but everything else has a much faster path

now, this is micro slow because it needs to allocate an uint16 on the heap to pass it as interface{} (until go 1.9)

so yeah, probably stop using zap.Any in hot paths for sure, but the reason isn't primarily "reflection" in this case

jcorbin

Meant to accept with last batch of comments.

jcorbin · 2017-05-04T17:55:01Z

bootstrap.go

 	if err != nil {
-		bootstrapLogger.Fatal("unable to initialize Arachne Logger", zap.Error(err))
+		bootstrapLogger.Error("unable to initialize Arachne Logger", zap.Error(err))
+		os.Exit(1)


why not bootstrapLogger.Fatal ?

Reverted to Fatal

jcorbin · 2017-05-04T17:57:01Z

collector/collector_test.go

-		zap.InfoLevel,
-		zap.DiscardOutput,
-	)
+	l, err := zap.NewDevelopment()


you may want to look into using zap's test support, but that's mostly for when you want to make assertions about "these sorts of logs happened"

jcorbin · 2017-05-04T17:59:28Z

glide.yaml

@@ -4,12 +4,16 @@ import:
  subpackages:
  - statsd
 - package: github.com/fatih/color
+- package: github.com/hashicorp/go-multierror


consider switching to go.uber.org/multierr ( http://go.uber.org/multierr )

jcorbin · 2017-05-04T18:02:19Z

internal/log/log.go

+		// Get the file size
+		stat, err := file.Stat()
+		if err != nil {
+			logger.Error("failed to read the FileInfo structure of the log file",


If you're going to return a multi-error, why do you also need to log about each of them? The caller should be able to log the multi-error after all this is done?

ResetLogFiles() now does not return anything. It's returned error was not previously used anyways.

jcorbin · 2017-05-04T18:03:58Z

internal/tcp/tcp.go

 			if err != nil {
-				logger.Debug("failed to send out", zap.String("flag", flag), srcZap, dstZap)
+				logger.Debug("failed to send out", lf...)


sounds like more of a warn or error log? but this was debug before, so not important I suppose

Converted to Error. Thanks.

Add support for zap v1.x APIs

ffa7249

Vasilis requested review from jcorbin and henridevieux April 29, 2017 00:26

Vasilis added 3 commits April 30, 2017 23:35

Update tests for zap v1.x

fea36a3

Fix uber-license commit version

42c70eb

Rename example function

33589ea

henridevieux reviewed May 1, 2017

View reviewed changes

henridevieux mentioned this pull request May 1, 2017

Address zap logger non-backwards compatibility issues #9

Closed

Vasilis added 3 commits May 2, 2017 14:32

Address comments

72f17f7

Show src address and port on received pkts

f606df8

Fix no stdout when no logsink provided

a784e97

billf approved these changes May 3, 2017

View reviewed changes

henridevieux approved these changes May 3, 2017

View reviewed changes

jcorbin reviewed May 3, 2017

View reviewed changes

Vasilis and others added 2 commits May 3, 2017 14:47

Create zap.Config in config phase and fix lint errors

cce77d5

Merge branch 'master' into zap

ec71340

jcorbin approved these changes May 4, 2017

View reviewed changes

Vasilis added 3 commits May 4, 2017 18:03

Add minor fixes

9d30cf3

Add minor fixes

f420b67

Fix line wrapping

3a77c8f

Vasilis merged commit 49fea49 into master May 5, 2017

Vasilis deleted the zap branch May 5, 2017 22:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for zap v1.x APIs #11

Add support for zap v1.x APIs #11

Vasilis commented Apr 29, 2017 •

edited

Loading

henridevieux May 1, 2017

Vasilis May 3, 2017

henridevieux May 1, 2017

billf May 3, 2017

Vasilis May 3, 2017

billf May 3, 2017

jcorbin May 3, 2017

Vasilis May 3, 2017

billf May 3, 2017

Vasilis May 3, 2017

henridevieux commented May 3, 2017

jcorbin left a comment

jcorbin May 3, 2017

Vasilis May 3, 2017

jcorbin May 3, 2017

Vasilis May 3, 2017

jcorbin May 3, 2017

billf May 3, 2017

Vasilis May 3, 2017 •

edited

Loading

Vasilis May 3, 2017

jcorbin May 3, 2017

jcorbin left a comment

jcorbin May 4, 2017

Vasilis May 4, 2017

jcorbin May 4, 2017

jcorbin May 4, 2017

jcorbin May 4, 2017

Vasilis May 5, 2017

jcorbin May 4, 2017

Vasilis May 5, 2017

Add support for zap v1.x APIs #11

Add support for zap v1.x APIs #11

Conversation

Vasilis commented Apr 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henridevieux commented May 3, 2017

jcorbin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vasilis May 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcorbin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vasilis commented Apr 29, 2017 •

edited

Loading

Vasilis May 3, 2017 •

edited

Loading