fix: set partial message metadata flags in log events #24
Conversation
PettitWesley
force-pushed
the
partial-message-flags
branch
2 times, most recently
from
3df15c7
to
e651f3c
Compare
|
CI is failing but I believe this is because it was not setup correctly: #25 (comment) |
|
@JoseVillalta Sure, pinged you with that CR |
|
Can you please update the PR to have more documentation about what the fix here is for and why are we making certain changes? Both PR description and commit message are very scant on details. Also, the code comments are sparse as well. It's hard to review these changes without that context. |
| @@ -334,6 +371,18 @@ func (l *Logger) Read( | |||
| } | |||
| } | |||
|
|
|||
| // generateRandomID is based on Docker | |||
| // GenerateRandomID: https://github.com/moby/moby/blob/bca8d9f2ce0d63e1490692917cde6273bc288bad/pkg/stringid/stringid.go#L40 | |||
There was a problem hiding this comment.
Why can we not just use that method? Why are we replicating it here?
There was a problem hiding this comment.
This is explained in my comment below this line... I can make it longer and more detailed to make it clearer. The Docker generate function does the rand generation in a loop, because it can't allow the ID to container only 0 - 9 (digits) because then it can't be used a container ID. We don't have that concern.
There was a problem hiding this comment.
Another reason to not import from docker is properly handling that error return in rand.Read
|
What would make it easier for readers to review this would be if you include a description of how you're solving the issue in the commit message (and/or code comments). You can repurpose this comment that you wrote in #23 as well: #23 (comment) |
PettitWesley
force-pushed
the
partial-message-flags
branch
from
e651f3c
to
197d48d
Compare
|
@aaithal Updated for your comments/Thanks for reviewing. |
|
looks much better, thanks! I have a minor comment. |
PettitWesley
force-pushed
the
partial-message-flags
branch
from
197d48d
to
388e0f7
Compare
|
Before we merge this, I want to run a test on my instance where I send it a very very large log, something like 10 MB, since the unit tests and functional tests and all of my previous real world tests all use logs denoted in KBs/bytes. |
|
Hi @PettitWesley I have tested your changes by running the Fargate Agent with your updated ECS Shim Logger code, and I am seeing the |
|
@garmikea Awesome... if those fields are present then its working... the only thing is to check that they make sense, the partial ordinals are ordered from 1.. N and the Nth log line has partial_last marked true. |
|
@garmikea there is also the negative case as well, that with small log lines these fields are not set. I will finish my final testing with very large log lines (multiple MBs) in a moment and I will also check the negative case as well. I think we are almost good to go with this change... |
|
@aaithal @JoseVillalta @garmikea I have completed manual test cases with the Fluent Bit implementation, it all works together: #23 (comment) |
This patch correctly set the partial metadata fields on the docker message structure. In EC2/vanilla Docker, this happens in the docker code. Those flags are expected to set. The docker message structure is then passed to the underlying log driver implementation which decides what to do with it. So for awslogs driver code for example, it does not use the partial fields at all, it ignores them and just sends the actual log line: https://github.com/moby/moby/blob/61404de7df1a6c75c2cbdc2c3ce226c95bebcaad/daemon/logger/awslogs/cloudwatchlogs.go#L601 Here is the relevant Docker code for setting these partial fields: https://github.com/moby/moby/blob/master/daemon/logger/copier.go#L114 The fix in this log driver wrapper is to set the partial fields correctly, but its still up to the underlying log driver implementation (which we import directly from the docker code base) to use or not use those fields. Signed-off-by: Wesley Pettit <wppttt@amazon.com>
Signed-off-by: Wesley Pettit <wppttt@amazon.com>
PettitWesley
force-pushed
the
partial-message-flags
branch
from
388e0f7
to
d39b51c
Compare
| if len(expectedPartialOrdinalSequence) > 0 && lines < len(expectedPartialOrdinalSequence) { | ||
| // check partial fields | ||
| require.Equal(t, expectedPartialOrdinalSequence[lines], msg.PLogMetaData.Ordinal) | ||
| if msg.PLogMetaData.Ordinal < lastPartialOrdinal { |
There was a problem hiding this comment.
you forgot to check that the partial field evaluates to true
Signed-off-by: Wesley Pettit wppttt@amazon.com
Issue #, if available:
#23
aws/containers-roadmap#1550
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.