Ultimate Access log filter #2988

mmatur · 2018-03-09T14:31:55Z

What does this PR do?

This PR is an implementation of #2875.

Motivation

Be able to customize accesslog like it was asked over PRs #2853 and #2397

Closes #2853, #2397, #2875

More

Added/updated tests
Added/updated documentation

ldez

LGTM

nmengin

LGTM

nmengin · 2018-03-12T11:28:45Z

types/logs.go

+func (f *FieldsHeadersNames) Set(value string) error {
+	fields := strings.Fields(value)
+
+	//f = make(FieldsHeadersNames)


Delete comment?

nmengin · 2018-03-13T10:12:46Z

docs/configuration/logs.md

+RetryAttempts
+```
+
+Deprecated way (before 1.4):


Maybe adding a DANGER for the deprecated fields, as described here

nmengin · 2018-03-13T10:16:32Z

types/logs.go

+// Set's argument is a string to be parsed to set the flag.
+// It's a comma-separated list, so we split it.
+func (f *FieldsHeadersNames) Set(value string) error {
+	fields := strings.Fields(value)


strings.Fields splits on white spaces and not a comma-separated list

nmengin · 2018-03-13T10:26:14Z

middlewares/accesslog/parser.go

+	submatch := regex.FindStringSubmatch(data)
+	result := make(map[string]string)
+
+	if len(submatch) > 13 {


Can you add a comment please?

m3co-code

I left mostly minor nit-picks and improvement suggestions. 👍 for the PR!

One thing I'm still convinced that we should do is to remove "redundant" fields from the access logs as described in #2875 (comment)

m3co-code · 2018-03-13T13:58:01Z

docs/configuration/logs.md

+
+  [accessLog.filters]
+
+  # statusCodes keep on only access logs with status code in the range


You could write it like: "keep only access logs with status codes in the specified range". I find it a bit easier to read.

m3co-code · 2018-03-13T13:59:24Z

docs/configuration/logs.md

+  # Optional
+  # Default: "keep"
+  #
+  defaultMode = "keep|drop"


I think we should try to provide working examples, so probably just showing "keep" as example. WDYT?

m3co-code · 2018-03-13T13:59:56Z

docs/configuration/logs.md

+
+  [accessLog.filters]
+
+  # statusCodes keep on only access logs with status code in the range


Same as comment below.

m3co-code · 2018-03-13T14:00:55Z

docs/configuration/logs.md

+    # Optional
+    # Default: "keep"
+    #
+    defaultMode = "drop|keep|redact"


Same as in the comment above.

m3co-code · 2018-03-13T15:39:20Z

integration/access_log_test.go

-	c.Assert(tokens[13], checker.Matches, `^\d+ms$`)
+	c.Assert(results[accesslog.OriginStatus], checker.Equals, v.code)
+	c.Assert(results[accesslog.RequestCount], checker.Equals, fmt.Sprintf("%d", i+1))
+	c.Assert(results[accesslog.FrontendName], checker.Matches, `^"?`+v.value+`.*$`)


Why is the frontend name stored in .value?

m3co-code · 2018-03-13T16:53:26Z

types/logs.go

+}
+
+// FieldsHeadersNames holds maps of fields with specific mode
+type FieldsHeadersNames map[string]string


Same here with the pluar form: FieldHeaderNames

m3co-code · 2018-03-13T16:56:34Z

types/logs.go

+	defaultKeep := true
+	if f != nil {
+		defaultKeep = checkFieldValue(f.DefaultMode, defaultKeep)
+		v, ok := f.Names[field]


You could merge this line with the next line.

An oversight here or do you disagree?

m3co-code · 2018-03-13T16:59:07Z

types/logs.go

+	}
+}
+
+// KeepHeader check if header need to be keep, drop or redact and return the status


Some grammar mistakes here, I think it should be: "KeepHeader checks if the headers need to be kept, dropped or redacted and returns the status".

m3co-code · 2018-03-13T17:01:43Z

types/logs_test.go

+		expected StatusCodes
+	}{
+		{
+			desc:     "One value should return StatusCodes of size 1",


I think the "should return StatusCodes ..." stuff can be dropped from the test descriptions.

I don't agree with that.

I'm not super hot about this one, I just think it's good to decrease optical "noise" and redundant explanations.

m3co-code · 2018-03-13T17:04:34Z

types/logs_test.go

+	}
+}
+
+func TestStatusCodesGet(t *testing.T) {


Do we really need to test the Getter? :)

Same here, this won't block my approval but adding ~30 lines of code to test one line that it is in fact super trivial and always does the same doesn't seem worth it.

mmatur · 2018-03-14T10:03:56Z

@marco-jantke Thanks for your review. I just push some fixes :)

m3co-code

I left some comments on the open points from my review, but they don't block my approval. Proceed with them as you think it's best.

So giving my approval, noice work! :-)

m3co-code · 2018-03-14T13:17:00Z

@mmatur one thing I just missed/forgot again. What do you think about the comment I brought up now two times to drop redundant fields? See #2988 (review)

danieladams456 · 2018-03-14T17:59:23Z

@marco-jantke I am in favor. Seems like that would be follow-up PR, right?

ldez · 2018-03-14T18:16:31Z

@marco-jantke @danieladams456 we think this be breaking for people who parse the log output.
For now, we will not change fields.

I added the need in this issue #2212

mmatur added kind/enhancement a new or improved feature. status/2-needs-review area/logs labels Mar 9, 2018

mmatur added this to the 1.6 milestone Mar 9, 2018

traefiker added area/middleware size/L labels Mar 9, 2018

mmatur force-pushed the feature/utimate-accesslogs branch from 7136639 to 49b0e13 Compare March 12, 2018 09:45

emilevauge mentioned this pull request Mar 12, 2018

Ultimate Access log filter #2875

Closed

mmatur force-pushed the feature/utimate-accesslogs branch from ff84671 to 6fba2e5 Compare March 13, 2018 14:28

ldez approved these changes Mar 13, 2018

View reviewed changes

nmengin approved these changes Mar 13, 2018

View reviewed changes

m3co-code suggested changes Mar 13, 2018

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Mar 13, 2018

mmatur removed the contributor/waiting-for-corrections label Mar 14, 2018

m3co-code approved these changes Mar 14, 2018

View reviewed changes

ldez added status/3-needs-merge and removed status/2-needs-review labels Mar 14, 2018

traefiker added the status/4-merge-in-progress label Mar 14, 2018

mmatur added 11 commits March 14, 2018 12:56

chore: create new types for accesslog filter

4f18578

chore: create new HTTPCodeRanges type

4381e07

chore: Set custom parser + default configuration

ded1bde

feat: create custom accesslog parser for tests

5eec788

feat: add accesslog filter and redact

6b9c68f

chore: remove unused library

c10ca33

fix integration tests

68c6f22

feat: separtion of concern

70f8a66

feat: improve tests

de51400

feat: add documentation

a126663

fix review

6eee25e

ldez and others added 6 commits March 14, 2018 12:56

refactor: review.

f0ef725

fix review on documentation

014ec56

Fix review

8d9247f

refactor: review.

6700be6

refactor: review.

add65dc

refactor: review.

7d3e9fd

traefiker force-pushed the feature/utimate-accesslogs branch from 7030deb to 7d3e9fd Compare March 14, 2018 12:56

traefiker removed the status/4-merge-in-progress label Mar 14, 2018

traefiker merged commit 8d46892 into traefik:master Mar 14, 2018

traefiker removed the status/3-needs-merge label Mar 14, 2018

mmatur deleted the feature/utimate-accesslogs branch March 14, 2018 13:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ultimate Access log filter #2988

Ultimate Access log filter #2988

mmatur commented Mar 9, 2018 •

edited by ldez

Loading

ldez left a comment

nmengin left a comment

nmengin Mar 12, 2018

nmengin Mar 13, 2018

nmengin Mar 13, 2018

nmengin Mar 13, 2018

m3co-code left a comment

m3co-code Mar 13, 2018

m3co-code Mar 13, 2018

m3co-code Mar 13, 2018

m3co-code Mar 13, 2018

m3co-code Mar 13, 2018

m3co-code Mar 13, 2018

m3co-code Mar 13, 2018

m3co-code Mar 14, 2018

m3co-code Mar 13, 2018

m3co-code Mar 13, 2018

ldez Mar 13, 2018

m3co-code Mar 14, 2018

m3co-code Mar 13, 2018

m3co-code Mar 14, 2018

mmatur commented Mar 14, 2018

m3co-code left a comment

m3co-code commented Mar 14, 2018

danieladams456 commented Mar 14, 2018

ldez commented Mar 14, 2018


		[accessLog.filters]

		# statusCodes keep on only access logs with status code in the range

Ultimate Access log filter #2988

Ultimate Access log filter #2988

Conversation

mmatur commented Mar 9, 2018 • edited by ldez Loading

What does this PR do?

Motivation

More

ldez left a comment

Choose a reason for hiding this comment

nmengin 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

m3co-code 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

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

mmatur commented Mar 14, 2018

m3co-code left a comment

Choose a reason for hiding this comment

m3co-code commented Mar 14, 2018

danieladams456 commented Mar 14, 2018

ldez commented Mar 14, 2018

mmatur commented Mar 9, 2018 •

edited by ldez

Loading