Repository navigation
Improve partial message support in logger - #35831
Conversation
|
There was a problem hiding this comment.
Ah, yes the space after json: should be removed
49802db to
bca9ea2
Compare
|
powerpc and z tests failures due to #35775 |
|
@thaJeztah Who do we know from splunk to review this? |
|
@outcoldman and @glennblock worked on the logging driver previously, but don't think they're still working for Splunk (but perhaps they know who to contact), and I saw @michaelwilde comment on one of the pull requests (he looks to be still with Splunk, so 👍) |
|
oh! saw https://github.com/splunk/docker-logging-plugin, so YOLO let me spam @bbourbie and @hyfather 😇 |
|
Also @mchene can find somebody for review. Btw, I believe HTTP Event Collector will just ignore Partial field in the event, see REST endpoint definition http://docs.splunk.com/Documentation/Splunk/7.0.1/RESTREF/RESTinput#services.2Fcollector |
|
Sorry just looking at this, was OOO -- hope everyone had a great holiday! 🎉 Each HTTP request to Splunk is treated separately, so there needs to be a unique identifier across all partial requests that docker sends so that Splunk can tell which lines need to be merged into a single event. Note that the Splunk endpoint that docker is making requests to could also be a load balancer and multiple HTTP requests carrying a partial line could be send to separate Splunk indexing servers. Getting a little Splunk specific here -- even if we change the plugin to add a line merging token to the first of the partial request to use the As far as sending |
|
@hyfather I am confused, looking on the documentation http://docs.splunk.com/Documentation/Splunk/7.0.1/RESTREF/RESTinput#services.2Fcollector, does not seem like HTTP Event Collector supports |
|
@outcoldman |
|
Maybe some missing context here, but I believe Docker Inc has been working with Splunk to get some kind of support added for this scenario (single line broken into multiple events due to huge lines) so that the moby splunk driver can deal with it properly. |
|
Looks like without unique ID attached to messages, this PR would not be useful for Splunk. So lets not merge this yet. @hyfather: Adding a unique 32 bit string ID to the messages can be done alongside partial log detection. Diff on top of this PR: https://gist.github.com/anusha-ragunathan/214b5c93ee89cb8342d101ada0f8bc63 |
bca9ea2 to
db9bf12
Compare
|
Failures on ppc and experimental due to #35914 |
|
@cpuguy83 @thaJeztah @outcoldman @hyfather @ripcurld0 I've updated the PR to include a unique |
|
I think for this to work we'd need a message ID which span across multiple partials and then an ordinal for each partial. |
|
My bad. Right now, |
|
@cpuguy83, @anusha-ragunathan -- yes a |
|
I don't think we can send along the number to expect, which indeed would be nice, unless we cache all the messages locally first, this would in turn bring us back to square one with the size of these log messages. |
5ada877 to
0db73b5
Compare
|
Keep in mind that generating an ID is not free.
I don't see why an ID would be needed for full messages unless there was some way to correlate that ID with something in the API.
Brian Goff
… On Apr 5, 2018, at 18:08, Sebastiaan van Stijn ***@***.***> wrote:
@thaJeztah commented on this pull request.
In daemon/logger/copier.go:
> @@ -112,7 +136,20 @@ func (c *Copier) copySrc(name string, src io.Reader) {
msg.Source = name
msg.Timestamp = time.Now().UTC()
msg.Line = append(msg.Line, buf[p:n]...)
- msg.Partial = true
+
+ // Generate unique partialID for first partial. Use it across partials.
+ // Initialize index for first partial. Increment it across partials.
+ if firstPartial {
+ partialid = stringid.GenerateRandomID()
+ ordinal = 0
+ firstPartial = false
+ }
+ ordinal++
+ setLogMetaData(msg, partialid, ordinal, false)
Let me try explaining my train of thought: if each message has a unique ID (irregardless of being partial or "full"), an aggregator could find all parts of a message by "grouping" by ID, and stitch them together based on ordinal.
The same logic could be used both for partial and full messages, so processing could be easier to handle if there's less differences between those
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
cpuguy83
left a comment
cpuguy83
left a comment
There was a problem hiding this comment.
Wondering if the message timestamp for partials should all be the same?
|
@cpuguy83 : Theoretically, they should be. But it doesnt serve any real purpose. |
Fair point. Still a bit on the fence for "partials" starting with ordinal 1, and "full" messages having ordinal 0 (#35831 (comment))
We're already using some state variables for partial messages; perhaps adding one for timestamp would work to address that? (even if it's theoretical, it would be "correct") |
|
@thaJeztah : I'll change the struct to a pointer to struct. That way, its nil for Full entries and avoids any confusion. |
0db73b5 to
5648a06
Compare
|
@cpuguy83 : Updated PR to have same timestamp for all partials PTAL. |
There was a problem hiding this comment.
Can we keep a PartialLogMetaData around to re-use... or usage a sync/pool?
I think just keeping a single one should work.
There was a problem hiding this comment.
Since the copy is sequential, I'm now simply using one allocated object over all msgs. Would that work for you?
There was a problem hiding this comment.
As discussed, let's just keep this as is for now since some log drivers handle messages asynchronously.
Thanks!
5648a06 to
448db5f
Compare
Docker daemon has a 16K buffer for log messages. If a message length exceeds 16K, it should be split by the logger and merged at the endpoint. This change adds `PartialLogMetaData` struct for enhanced partial support - LastPartial (bool) : indicates if this is the last of all partials. - ID (string) : unique 32 bit ID. ID is same across all partials. - Ordinal (int starts at 1) : indicates the position of msg in the series of partials. Also, the timestamps across partials in the same. Signed-off-by: Anusha Ragunathan <anusha.ragunathan@docker.com>
448db5f to
0b4b0a7
Compare
|
These are known flaky's, tracked through #36501 This one looks to have become flaky again (tracking issue: #32673). I attached the daemon logs for that test |
|
splunk-logging-plugin is already using the Partial flag now to reassemble messages. This change is a breaking change for our plugins. I assume this change would go in a major release? |
|
@sharonx this is an internal only change and opens up built-in drivers to support the additional parameters. Plugins will require some extra work to expose this data to the plugin, but there should be no change to the data currently being sent to the plugin. When the work is done to bring plugins up to speed with thos change it will be purely additive. |
|
Hi All, I am using splunk logging driver to send logs to splunk, and noticed that, when splunk-format is inline, we are able to send messages >10K in size, but when sending them in raw format, message gets truncated at 4K, is that expected? Can someone provide any pointers on this issue, if this is not the right place? |
|
@kyaparla : Messages get split at 16K. So what you are observing is not expected. |
|
Thanks @anusha-ragunathan, opened this issue Splunk logging driver truncates messages at 4K size when splunk-format is set to raw #37096 |
Improve partial message support in logger and use it in Splunk.
Docker daemon has a 16K buffer for log messages. If a message length
exceeds 16K, it should be split by the logger and merged at the
endpoint.
This change adds
LogMetaDatastruct for enhanced partial supportIsPartial (bool) : Indicates if this message is partial
LastPartial (bool) : indicates if this is the last of all partials.
ID (string) : unique 32 bit ID. ID is same across all partials.
Ordinal (int starts at 1) : indicates the position of msg in the series of partials.
Signed-off-by: Anusha Ragunathan anusha.ragunathan@docker.com