feat: support max-buffer-size for AWSLogs driver#26396
feat: support max-buffer-size for AWSLogs driver#26396mergify[bot] merged 6 commits intoaws:mainfrom
Conversation
aws-cdk-automation
left a comment
There was a problem hiding this comment.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.
| * store messages. This parameter is not valid with | ||
| * AwsLogDriverMode.BLOCKING. |
There was a problem hiding this comment.
Since this is not valid with blocking mode, should I add some sort of check? How does that work? Can someone help me with how to do that- does it go in the bind method and what do I make it do/throw/return on failure?
There was a problem hiding this comment.
Something like? =>
if (this.props.maxBufferSize != "" && this.props.mode != AwsLogDriverMode. NON_BLOCKING) {
throw new Error('max-buffer-size requires non-blocking mode')
}
There was a problem hiding this comment.
Yes i was about to ask for the same thing. It will be great if we can perform a check in L2.
Maybe
if (this.props.maxBufferSize != "" && this.props.mode === AwsLogDriverMode.BLOCKING) { throw new Error('max-buffer-size is not valid with blocking mode') }which is more related to "This parameter is not valid with AwsLogDriverMode.BLOCKING.", unless you want to change the comment msg to "this parameter is only valid with non-blocking mode".
46af877 to
edcaa85
Compare
|
Integ test results showing that the buffer size was set in the task def: 
|
250f0d4 to
872d850
Compare
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
5eaf26d to
189dbf1
Compare
iamhopaul123
left a comment
There was a problem hiding this comment.
LGTM overall! Can we also add a unit test for the new error scenario?
3587927 to
f95a29c
Compare
|
Added unit test |
befc967 to
db80eae
Compare
497e593 to
53a1263
Compare
2212738 to
633b1bf
Compare
| throw new Error('Cannot specify both `logGroup` and `logRetentionDays`.'); | ||
| } | ||
|
|
||
| if (props.maxBufferSize !== undefined && props.mode !== AwsLogDriverMode.NON_BLOCKING) { |
There was a problem hiding this comment.
No that we're using Size as the type we can just use:
if (props.maxBufferSize && props.mode !== AwsLogDriverMode.NON_BLOCKING
| retention: this.props.logRetention || Infinity, | ||
| }); | ||
|
|
||
| var maxBufferSize = undefined; |
There was a problem hiding this comment.
We really shouldn't use var here. Honestly we can probably just simplify this whole statement with a ternary operator:
const maxBufferSize = this.props.maxBufferSize
? `${this.props.maxBufferSize.toBytes({ rounding: SizeRoundingBehavior.FLOOR })}b`
: undefined;
colifran
left a comment
There was a problem hiding this comment.
A few more minor comments and then this will be good to go.
|
|
||
| }); | ||
|
|
||
| test('throws error when specifying maxBufferSize and blocking mode', () => { |
There was a problem hiding this comment.
Can you add one more unit test to validate that this will throw when maxBufferSize is used and no blocking mode is specified that way we're validating that we can't specify maxBufferSize with the default driver mode
Signed-off-by: Wesley Pettit <wppttt@amazon.com>
633b1bf to
59b51c7
Compare
colifran
left a comment
There was a problem hiding this comment.
@PettitWesley Looks great. Thanks for your work on this!
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
When mode is set to
non-blocking, logs are stored in a buffer. The settingmax-buffer-sizecontrols the size of this buffer. Being able to set this buffer is very important, the larger the buffer, the less likely there is to be log loss.Recently I performed benchmarking of
non-blockingmode, and found that the default buffer size is not sufficient to prevent log loss: moby/moby#45999We're planning to run an education campaign to ensure ECS customers understand the risk with the default
blockingmode, and the risk withnon-blockingmode with the default buffer size. Therefore, we need folks to be able to control this setting in their CDK.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license