Add access log filter for retry attempts #3042
Conversation
mjeri
force-pushed
the
add-accesslog-retry-filter
branch
from
538af4f
to
5551bac
Compare
ldez
added
kind/enhancement
| @@ -357,6 +357,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.
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.
That is right. Now when you specify Filters but nothing inside it will remove everything. Isn't that the kind of logical behavior? The filters are OR-connected and when you specify multiple (status codes and retry attempts) than the result is more than when you specify only one of those. WDYT?
There was a problem hiding this comment.
Thanks for your response @marco-jantke
When we specify nothing in Filters, we need to keep all access logs. And when something is specified in Filters we need to keep only access logs matching at least one filter
There was a problem hiding this comment.
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.
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
types/types_test.go
Outdated
| test := test | ||
| testName := fmt.Sprintf("%q contains %d", test.strBlocks, test.statusCode) | ||
| t.Run(testName, func(t *testing.T) { | ||
| httpCodeRanges, err := NewHTTPCodeRanges(test.strBlocks) |
There was a problem hiding this comment.
Could you please run these test cases in parallel
| # | ||
| # Optional | ||
| # Default: [] | ||
| # | ||
| statusCodes = ["200", "300-302"] | ||
|
|
||
| # retryAttempts keep access logs when at least one retry happened |
There was a problem hiding this comment.
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 the statusCodes ranges.
Thus users can understand that filters are OR-connected 😉
There was a problem hiding this comment.
@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?
traefiker
force-pushed
the
add-accesslog-retry-filter
branch
from
0df7ce1
to
7a3476b
Compare
ldez
changed the title
This PR implements #3020.