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

Add JSON as access logging format #1669

Merged
merged 1 commit into from
Jun 6, 2017
Merged

Conversation

rjshep
Copy link
Contributor

@rjshep rjshep commented May 25, 2017

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.

Copy link
Contributor

@ldez ldez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few comments.

@@ -456,6 +457,10 @@ func NewTraefikDefaultPointersConfiguration() *TraefikConfiguration {
defaultDynamoDB.TableName = "traefik"
defaultDynamoDB.Watch = true

var defaultAccessLog types.AccessLog
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a comment before this line // default AccessLog

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can write simply:

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

@@ -42,7 +42,8 @@ type GlobalConfiguration struct {
GraceTimeOut flaeg.Duration `short:"g" description:"Duration to give active requests a chance to finish during hot-reload"`
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"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add // Deprecated at the end of the line.

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]",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

var formatter logrus.Formatter

switch config.Format {
case "common":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can create a constant for "common"

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can create a constant for "json"

tmpDir, logfilePath := doLogging(t, "json")
defer os.RemoveAll(tmpDir)

var jsonDataIf interface{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

tmpDir, logfilePath := doLogging(t, "common")
defer os.RemoveAll(tmpDir)

if logdata, err := ioutil.ReadFile(logfilePath); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

rjshep commented Jun 2, 2017

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
Copy link
Contributor

ldez commented Jun 2, 2017

@rjshep you need to resolve conflicts for activate Semaphore.

Copy link
Contributor

@ldez ldez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😃

}

func TestLoggerJSON(t *testing.T) {
tmpDir, logfilePath := doLogging(t, JSONFormat)
Copy link
Contributor

@ldez ldez Jun 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

beware to use camelCase. logfilePath -> logFilePath

tmpDir, logfilePath := doLogging(t, JSONFormat)
defer os.RemoveAll(tmpDir)

var jsonDataIf interface{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

assert.Equal(t, fmt.Sprintf("%s:%d", testHostname, testPort), jsonData[ClientAddr], printLogdataJSON(jsonData))
assert.Equal(t, "info", jsonData["level"], printLogdataJSON(jsonData))
assert.Equal(t, "", jsonData["msg"], printLogdataJSON(jsonData))
assert.True(t, jsonData[RequestCount].(float64) > 0, printLogdataJSON(jsonData))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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]",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@ldez ldez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job again! 👍 👍 👍

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

types/types.go Outdated
// AccessLog holds the configuration settings for the access logger (middlewares/accesslog).
type AccessLog struct {
FilePath string `json:"file,omitempty" description:"Access log file path"`
Format string `json:"format,omitempty" description:"Access log format: json | common (default: common)"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

@emilevauge emilevauge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @rjshep!
Great job ❤️
LGTM

@ldez ldez force-pushed the access-logging-json branch 4 times, most recently from 2c6e016 to d5004e3 Compare June 6, 2017 13:46
@ldez ldez merged commit a9216e2 into traefik:master Jun 6, 2017
@altovedo altovedo deleted the access-logging-json branch September 12, 2018 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/logs kind/enhancement a new or improved feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants