Added duration filter for logs #3463

rodrigodiez · 2018-06-09T19:12:02Z

What does this PR do?

Added duration filter for logs

Motivation

Fixes #3455

More

Added/updated tests
Added/updated documentation

Additional Notes

First PR so please be gentle 😄 I guess there may be some debate about naming

m3co-code

Hi @rodrigodiez,

great to see your first PR on Traefk and thanks for the contribution! I left a couple of comments, the PR looks very good in general!

One general thing I wanted to suggest is that we re-name the parameter to minDuration. I think that would make it more explicit what the parameter does. WDYT?

m3co-code · 2018-06-10T20:24:28Z

docs/configuration/logs.md

@@ -159,6 +161,13 @@ format = "json"
  # Default: false
  #
  retryAttempts = true
+
+  # duration keep access logs when request took more than the specified duration


Nit-pick: can we add a : after the filter name for this and the previous explanations? E.g. # duration: keep access logs...

I'm not entirely sure but I think it should also be when a request took longer than ... Maybe @dtomcej can help out here :-)

m3co-code · 2018-06-10T20:28:55Z

middlewares/accesslog/logger.go

@@ -322,6 +324,10 @@ func (l *LogHandler) keepAccessLog(statusCode, retryAttempts int) bool {
 		return true
 	}

+	if l.config.Filters.Duration > 0 && (parse.Duration(duration) > l.config.Filters.Duration) {


I think we can skip the first check on the Duration > 0. If it's 0 the duration will be larger anyway.

This check is needed to ensure the filter has been set up. Otherwise the log will be kept 100% of the times since Duration zero value is 0

Yes, you're right, now I got it. Thanks for explaining!

m3co-code · 2018-06-10T21:14:16Z

types/logs.go

-	RetryAttempts bool        `json:"retryAttempts,omitempty" description:"Keep access logs when at least one retry happened" export:"true"`
+	StatusCodes   StatusCodes    `json:"statusCodes,omitempty" description:"Keep access logs with status codes in the specified range" export:"true"`
+	RetryAttempts bool           `json:"retryAttempts,omitempty" description:"Keep access logs when at least one retry happened" export:"true"`
+	Duration      parse.Duration `json:"duration,omitempty" description:"Keep access logs with response times above the specific duration" export:"true"`


Is it possible to use time.Duration as type as well? I'm not so much into the flaeg stuff. Maybe @ldez knows which type is best to use here. I saw time.Duration as example in the flaeg README, but on other places we use flaeg.Duration mostly. I'm not sure which one we should use.

flaeg.Duration is replaced by parse.Duration.

parse.Duration is the good type for the configuration.

rodrigodiez

Hey @marco-jantke, thanks for reviewing and for your comments! They make total sense.

I wrote a response to one of your comments explaining why we need a > 0 check but other than that I've adressed everything 👍

rodrigodiez · 2018-06-11T06:02:35Z

middlewares/accesslog/logger.go

@@ -322,6 +324,10 @@ func (l *LogHandler) keepAccessLog(statusCode, retryAttempts int) bool {
 		return true
 	}

+	if l.config.Filters.Duration > 0 && (parse.Duration(duration) > l.config.Filters.Duration) {


This check is needed to ensure the filter has been set up. Otherwise the log will be kept 100% of the times since Duration zero value is 0

m3co-code

LGTM, nice job 👏

rodrigodiez · 2018-06-11T07:32:25Z

thanks! And please keep adding begginer tasks, they are so valuable!

…ponse-time

dtomcej

Very close!

dtomcej · 2018-06-11T14:12:19Z

docs/configuration/logs.md

@@ -146,19 +148,26 @@ format = "json"

  [accessLog.filters]

-  # statusCodes keep access logs with status codes in the specified range
+  # statusCodes: keep access logs with status codes in the specified range


statusCodes: keep access logs with status codes within the specified range

that description has not changed as result of this PR, should we block the feature because of it? Honestly asking. Sounds like a separate issue (bettering the existing doc)

Ah, sorry, it's early here. You are correct :)

I see! :D I was honestly asking, there may be a policy in place in the project to better things as they are spotted, did not know. Thx for your comments!

rodrigodiez · 2018-06-11T14:31:48Z

docs/configuration/logs.md

  #
  # Optional
  # Default: []
  #
  statusCodes = ["200", "300-302"]

-  # retryAttempts keep access logs when at least one retry happened
+  # retryAttempts: keep access logs when at least one retry happened


Same coment as above :)

rodrigodiez · 2018-06-11T14:31:24Z

docs/configuration/logs.md

  #
  # Optional
  # Default: false
  #
  retryAttempts = true
+
+  # minDuration: keep access logs when request took longer than the specified duration


I find confusing to use present form (takes longer) and past form (has ocurred) in the descriptions across filters.

We should probably attempt to get some consistency. Ideas?

rodrigodiez · 2018-06-11T14:31:24Z

docs/configuration/logs.md

  #
  # Optional
  # Default: false
  #
  retryAttempts = true
+
+  # minDuration: keep access logs when request took longer than the specified duration


I find confusing to use present form (takes longer) and past form (has ocurred) in the descriptions across filters.

We should probably attempt to get some consistency. Ideas?

rodrigodiez · 2018-06-11T14:31:48Z

docs/configuration/logs.md

  #
  # Optional
  # Default: []
  #
  statusCodes = ["200", "300-302"]

-  # retryAttempts keep access logs when at least one retry happened
+  # retryAttempts: keep access logs when at least one retry happened


Same coment as above :)

dtomcej

LGTM

mmatur

Nice first PR.
LGTM 👏

Added duration filter for logs

abf3857

traefiker added area/logs area/middleware size/S status/0-needs-triage labels Jun 9, 2018

This comment has been minimized.

Sign in to view

rodrigodiez added 2 commits June 9, 2018 20:50

Using flaeg.Duration instead of time.Duration

4cff8f3

Fixed some tests

ae74970

ldez added status/2-needs-review and removed status/0-needs-triage labels Jun 9, 2018

m3co-code suggested changes Jun 10, 2018

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Jun 10, 2018

Addressed PR review comments

c42ad2b

rodrigodiez commented Jun 11, 2018

View reviewed changes

m3co-code approved these changes Jun 11, 2018

View reviewed changes

traefiker removed the contributor/waiting-for-corrections label Jun 11, 2018

Merge branch 'master' into feature/add-access-log-filter-based-on-res…

ad6983f

…ponse-time

dtomcej suggested changes Jun 11, 2018

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Jun 11, 2018

rodrigodiez commented Jun 11, 2018

View reviewed changes

dtomcej approved these changes Jun 11, 2018

View reviewed changes

traefiker removed the contributor/waiting-for-corrections label Jun 11, 2018

mmatur added this to the 1.7 milestone Jun 11, 2018

ldez requested a review from mmatur June 11, 2018 14:49

mmatur approved these changes Jun 11, 2018

View reviewed changes

mmatur added kind/enhancement a new or improved feature. status/3-needs-merge labels Jun 11, 2018

mmatur removed the status/2-needs-review label Jun 11, 2018

traefiker merged commit 1fbf5b8 into traefik:master Jun 11, 2018

traefiker removed the status/3-needs-merge label Jun 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added duration filter for logs #3463

Added duration filter for logs #3463

rodrigodiez commented Jun 9, 2018 •

edited by ldez

Loading

This comment has been minimized.

This comment has been minimized.

m3co-code left a comment

m3co-code Jun 10, 2018

m3co-code Jun 10, 2018

m3co-code Jun 10, 2018

rodrigodiez Jun 11, 2018

m3co-code Jun 11, 2018

m3co-code Jun 10, 2018

ldez Jun 10, 2018

rodrigodiez left a comment

rodrigodiez Jun 11, 2018

m3co-code left a comment

rodrigodiez commented Jun 11, 2018

dtomcej left a comment

dtomcej Jun 11, 2018

rodrigodiez Jun 11, 2018 •

edited

Loading

dtomcej Jun 11, 2018

rodrigodiez Jun 11, 2018

rodrigodiez Jun 11, 2018

rodrigodiez Jun 11, 2018

rodrigodiez Jun 11, 2018

rodrigodiez Jun 11, 2018

dtomcej left a comment

mmatur left a comment

Added duration filter for logs #3463

Added duration filter for logs #3463

Conversation

rodrigodiez commented Jun 9, 2018 • edited by ldez Loading

What does this PR do?

Motivation

More

Additional Notes

This comment has been minimized.

This comment has been minimized.

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

rodrigodiez left a 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

rodrigodiez commented Jun 11, 2018

dtomcej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigodiez Jun 11, 2018 • 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

dtomcej left a comment

Choose a reason for hiding this comment

mmatur left a comment

Choose a reason for hiding this comment

rodrigodiez commented Jun 9, 2018 •

edited by ldez

Loading

rodrigodiez Jun 11, 2018 •

edited

Loading