Handle long log messages correctly on SizedLogger#41909
Merged
Conversation
samuelkarp
approved these changes
Jan 20, 2021
jahkeup
approved these changes
Jan 20, 2021
Member
|
Loggers that implement BufSize() (e.g. awslogs) uses the method to tell Copier about the maximum log line length. However loggerWithCache and RingBuffer hide the method by wrapping loggers. As a result, Copier uses its default 16KB limit which breaks log lines > 16kB even the destinations can handle that. This change implements BufSize() on loggerWithCache and RingBuffer to make sure these logger wrappes don't hide the method on the underlying loggers. Fixes moby#41794. Signed-off-by: Kazuyoshi Kato <[email protected]>
kzys
force-pushed
the
41794-sized-logger
branch
from
January 21, 2021 00:44
1e8dae0 to
bb11365
Compare
thaJeztah
approved these changes
Jan 21, 2021
thaJeztah
left a comment
thaJeztah
left a comment
Member
There was a problem hiding this comment.
LGTM
Test failures are unrelated; the updated test passed;
<testcase classname="github.com/docker/docker/daemon/logger" name="TestCopierWithSized/as_is" time="0.000000"></testcase>
<testcase classname="github.com/docker/docker/daemon/logger" name="TestCopierWithSized/With_RingLogger" time="0.000000"></testcase>
<testcase classname="github.com/docker/docker/daemon/logger" name="TestCopierWithSized" time="0.010000"></testcase>| return name | ||
| } | ||
|
|
||
| // BufSize returns the maximum bytes CloudWatch can handle. |
Member
There was a problem hiding this comment.
Interesting that the linter didn't fail before this PR 🤔
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Loggers that implement BufSize() (e.g. awslogs) uses the method to
tell Copier about the maximum log line length. However loggerWithCache
and RingBuffer hide the method by wrapping loggers.
As a result, Copier uses its default 16KB limit which breaks log
lines > 16kB even the destinations can handle that.
This change implements BufSize() on loggerWithCache and RingBuffer to
make sure these logger wrappes don't hide the method on the underlying
loggers.
Fixes #41794.
Signed-off-by: Kazuyoshi Kato [email protected]
- What I did
I've added BufSize() to loggerWithCache and RingLogger to make Copier aware about the size limit of the underlying loggers.
- How I did it
I've added the method to the structs.
- How to verify it
I built Docker and tested the change with
log-splitcontainer image I made.The Dockerfile for the image is below.
I also added new unit tests on Copier.
- Description for the changelog
Fix loggers to make sure they don't break long lines when the destinations' limits allow.
- A picture of a cute animal (not mandatory but encouraged)
(From https://www.zoo.org/animals/digital)