Add JSON as access logging format #1669

rjshep · 2017-05-25T12:04:36Z

Now that access logging uses logrus, (#1647), add JSON as a supported format.

The existing accessLogFile config entry is deprecated in favour of a accessLog section, allowing both the filePath and format to be specified. The common log format is retained as default.

To format JSON the standard logrus JSON formatter is used.

ldez

Few comments.

ldez · 2017-06-02T09:48:37Z

server/configuration.go

 	defaultDynamoDB.TableName = "traefik"
 	defaultDynamoDB.Watch = true

+	var defaultAccessLog types.AccessLog


add a comment before this line // default AccessLog

you can write simply:

defaultAccessLog := types.AccessLog{ Format: "common", FilePath: "", }

ldez · 2017-06-02T09:49:15Z

server/configuration.go

 	Debug                     bool                    `short:"d" description:"Enable debug mode"`
 	CheckNewVersion           bool                    `description:"Periodically check if a new version has been released"`
-	AccessLogsFile            string                  `description:"Access logs file"`
+	AccessLogsFile            string                  `description:"(Deprecated) Access logs file"`


add // Deprecated at the end of the line.

ldez · 2017-06-02T10:59:22Z

middlewares/accesslog/logger_test.go

+	return fmt.Sprintf(
+		"\nExpected: %s\n"+
+			"Actual:   %v",
+		"map[ClientAddr:TestHost:8181 Duration:50771 FrontendName:testFrontend RequestLine:POST testpath HTTP/0.0 request_Referer:testReferer request_User-Agent:testUserAgent RequestCount:2 RequestHost:TestHost StartLocal:2017-05-25T11:27:28.797663791+01:00 BackendURL:http://127.0.0.1/testBackend ClientHost:TestHost DownstreamStatusLine:123  Overhead:50771 RequestAddr:TestHost DownstreamStatus:123 OriginStatus:123 RequestPort:- RequestProtocol:HTTP/0.0 ClientPort:8181 OriginContentSize:12 RequestMethod:POST ClientUsername:TestUser RequestPath:testpath level:info msg: time:2017-05-25T11:27:28+01:00 DownstreamContentSize:12 StartUTC:2017-05-25T10:27:28.797663791Z]",


I'm in trouble with this part, I must think if another way exists.

you can use the JSON content instead of the serialization of a map.

ldez · 2017-06-02T11:00:12Z

middlewares/accesslog/logger.go

+	var formatter logrus.Formatter
+
+	switch config.Format {
+	case "common":


I think you can create a constant for "common"

ldez · 2017-06-02T11:00:21Z

middlewares/accesslog/logger.go

+	switch config.Format {
+	case "common":
+		formatter = new(CommonLogFormatter)
+	case "json":


I think you can create a constant for "json"

ldez · 2017-06-02T11:00:57Z

middlewares/accesslog/logger_test.go

+	tmpDir, logfilePath := doLogging(t, "json")
+	defer os.RemoveAll(tmpDir)
+
+	var jsonDataIf interface{}


you can simplify like that:

tmpDir, logfilePath := doLogging(t, "json") defer os.RemoveAll(tmpDir) logdata, err := ioutil.ReadFile(logfilePath) require.NoError(t, err) fmt.Println(string(logdata)) jsonData := make(map[string]interface{}) err = json.Unmarshal(logdata, &jsonData) require.NoError(t, err) require.Equal(t, 28, len(jsonData), printLogdataJSON(jsonData)) assert.Equal(t, testHostname, jsonData[RequestHost], printLogdataJSON(jsonData))

ldez · 2017-06-02T11:02:33Z

middlewares/accesslog/logger_test.go

+	tmpDir, logfilePath := doLogging(t, "common")
+	defer os.RemoveAll(tmpDir)
+
+	if logdata, err := ioutil.ReadFile(logfilePath); err != nil {


you can simplify like that:

logdata, err := ioutil.ReadFile(logfilePath) require.NoError(t, err) tokens, err := shellwords.Parse(string(logdata)) require.NoError(t, err) assert.Equal(t, 14, len(tokens), printLogdata(logdata))

rjshep · 2017-06-02T13:29:34Z

Hi @ldez I've made the changes requested, however it doesn't look like the travis and semaphore checks are triggering. Has something changed - is it a case of closing and reopening the PR as was needed a while ago?

ldez · 2017-06-02T17:23:51Z

@rjshep you need to resolve conflicts for activate Semaphore.

ldez

It's hard to me to explains all changes, also you can look at ldez@5154374

If you have some questions, don't hesitate to ask me 😃

ldez · 2017-06-03T15:42:47Z

middlewares/accesslog/logger_test.go

beware to use camelCase. logfilePath -> logFilePath

ldez · 2017-06-03T15:43:27Z

middlewares/accesslog/logger_test.go

can be simplify:

logData, err := ioutil.ReadFile(logFilePath) require.NoError(t, err) jsonData := make(map[string]interface{}) err = json.Unmarshal(logData, &jsonData) require.NoError(t, err)

ldez · 2017-06-03T15:46:21Z

middlewares/accesslog/logger_test.go

assert.True with comparisons don't really provide a good message when they failed.
Prefer assert.NotZero(t, jsonData[RequestCount].(float64))

ldez · 2017-06-03T15:47:32Z

middlewares/accesslog/logger_test.go

+	return fmt.Sprintf(
+		"\nExpected: %s\n"+
+			"Actual:   %v",
+		"map[ClientAddr:TestHost:8181 Duration:50771 FrontendName:testFrontend RequestLine:POST testpath HTTP/0.0 request_Referer:testReferer request_User-Agent:testUserAgent RequestCount:2 RequestHost:TestHost StartLocal:2017-05-25T11:27:28.797663791+01:00 BackendURL:http://127.0.0.1/testBackend ClientHost:TestHost DownstreamStatusLine:123  Overhead:50771 RequestAddr:TestHost DownstreamStatus:123 OriginStatus:123 RequestPort:- RequestProtocol:HTTP/0.0 ClientPort:8181 OriginContentSize:12 RequestMethod:POST ClientUsername:TestUser RequestPath:testpath level:info msg: time:2017-05-25T11:27:28+01:00 DownstreamContentSize:12 StartUTC:2017-05-25T10:27:28.797663791Z]",


you can use the JSON content instead of the serialization of a map.

ldez

Great job again! 👍 👍 👍

I have added a small commit.
I left a last comments but LGTM.

ldez · 2017-06-05T11:02:19Z

types/types.go

Flaeg (the lib we use for the CLI) provide automatically the default value in display if a configuration struct have been initialize with a value like in your case.

// default AccessLog defaultAccessLog := types.AccessLog{ Format: accesslog.CommonFormat, FilePath: "", }

You can remove (default: common)

emilevauge

Thanks a lot @rjshep!
Great job ❤️
LGTM

ldez added area/logs kind/enhancement a new or improved feature. status/1-needs-design-review labels May 25, 2017

ldez requested review from emilevauge and timoreimann May 25, 2017 13:06

ldez added this to the 1.4 milestone May 25, 2017

ldez added status/2-needs-review and removed status/1-needs-design-review labels May 30, 2017

ldez suggested changes Jun 2, 2017

View reviewed changes

rjshep force-pushed the access-logging-json branch from 25b237f to 83715f0 Compare June 2, 2017 13:26

ldez suggested changes Jun 3, 2017

View reviewed changes

ldez force-pushed the access-logging-json branch from 3a134d6 to ed34b9a Compare June 5, 2017 11:10

ldez approved these changes Jun 5, 2017

View reviewed changes

ldez mentioned this pull request Jun 5, 2017

[Feature Request] ElasticSearch 5.x compatible logs #1710

Closed

emilevauge approved these changes Jun 6, 2017

View reviewed changes

ldez force-pushed the access-logging-json branch 4 times, most recently from 2c6e016 to d5004e3 Compare June 6, 2017 13:46

Add JSON as access logging format

0320f0b

ldez force-pushed the access-logging-json branch from d5004e3 to 0320f0b Compare June 6, 2017 14:08

ldez merged commit a9216e2 into traefik:master Jun 6, 2017

ldez removed the status/2-needs-review label Jun 6, 2017

altovedo deleted the access-logging-json branch September 12, 2018 08:59

Uh oh!

Add JSON as access logging format #1669

Add JSON as access logging format #1669

Uh oh!

Conversation

rjshep commented May 25, 2017

Uh oh!

ldez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjshep commented Jun 2, 2017

Uh oh!

ldez commented Jun 2, 2017

Uh oh!

ldez left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldez Jun 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emilevauge left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ldez left a comment •

edited

Loading

ldez Jun 3, 2017 •

edited

Loading