-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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 access log filter for retry attempts #3042
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -335,6 +335,15 @@ func TestNewLogHandlerOutputStdout(t *testing.T) { | |
}, | ||
expectedLog: `TestHost - TestUser [13/Apr/2016:07:14:19 -0700] "POST testpath HTTP/0.0" 123 12 "testReferer" "testUserAgent" 23 "testFrontend" "http://127.0.0.1/testBackend" 1ms`, | ||
}, | ||
{ | ||
desc: "default config with empty filters", | ||
config: &types.AccessLog{ | ||
FilePath: "", | ||
Format: CommonFormat, | ||
Filters: &types.AccessLogFilters{}, | ||
}, | ||
expectedLog: `TestHost - TestUser [13/Apr/2016:07:14:19 -0700] "POST testpath HTTP/0.0" 123 12 "testReferer" "testUserAgent" 23 "testFrontend" "http://127.0.0.1/testBackend" 1ms`, | ||
}, | ||
{ | ||
desc: "Status code filter not matching", | ||
config: &types.AccessLog{ | ||
|
@@ -357,6 +366,17 @@ func TestNewLogHandlerOutputStdout(t *testing.T) { | |
}, | ||
expectedLog: `TestHost - TestUser [13/Apr/2016:07:14:19 -0700] "POST testpath HTTP/0.0" 123 12 "testReferer" "testUserAgent" 23 "testFrontend" "http://127.0.0.1/testBackend" 1ms`, | ||
}, | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I add this test case {
desc: "Default config",
config: &types.AccessLog{
FilePath: "",
Format: CommonFormat,
Filters: &types.AccessLogFilters{},
},
expectedLog: `TestHost - TestUser [13/Apr/2016:07:14:19 -0700] "POST testpath HTTP/0.0" 123 12 "testReferer" "testUserAgent" 23 "testFrontend" "http://127.0.0.1/testBackend" 1ms`,
}, The default response has changed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is right. Now when you specify There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for your response @marco-jantke There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As said before I think that's not logical. And "optimizing" for a use-case where someone would specify an empty filters config doesn't make too much sense. However, I implemented it now as you requested in 6ca142b. PTAL and let me know what you think. I didn't see a nicer option for detecting an empty configuration except validating each config value one by one to check whether it's the zero value. Note that with this approach we have to extend this method each time a new option gets added. I'm open to improvement ideas. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for your correction @marco-jantke I am pretty agree with you but I think that it is not the aim of this PR to change the default behavior. To me you have choose the best option to detect an empty configuration. We should maybe do an other PR to improve this method to avoid to extend it every time we add a new option |
||
desc: "Retry attempts filter matching", | ||
config: &types.AccessLog{ | ||
FilePath: "", | ||
Format: CommonFormat, | ||
Filters: &types.AccessLogFilters{ | ||
RetryAttempts: true, | ||
}, | ||
}, | ||
expectedLog: `TestHost - TestUser [13/Apr/2016:07:14:19 -0700] "POST testpath HTTP/0.0" 123 12 "testReferer" "testUserAgent" 23 "testFrontend" "http://127.0.0.1/testBackend" 1ms`, | ||
}, | ||
{ | ||
desc: "Default mode keep", | ||
config: &types.AccessLog{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a specify that this option will keep
keep access logs when at least one retry happened
even if the status code is not in thestatusCodes
ranges.Thus users can understand that filters are OR-connected 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nmengin I added now a sentence at the beginning of the segment, so that we don't have to repeat that sentence on every filter we have :D
Can you have a look and let me know whether it would have helped you?