Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat: ensure that log batches don't exceed 1MiB in logSender#12048

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

spikecurtis
Copy link
Contributor

@spikecurtisspikecurtis commentedFeb 7, 2024
edited
Loading

Protobuf messages are designed to be about 1MiB or less, so this PR ensures that even if we get a lot of logs or logs with huge payloads we don't send more than 1MiB at a time.

We straight up drop logs that themselves exceed 1MiB, and batch up logs so that each request is less than 1MiB if we have a lot of small logs.

@spikecurtisGraphite App
Copy link
ContributorAuthor

spikecurtis commentedFeb 7, 2024
edited
Loading

agent/logs.go Outdated
const (
flushInterval = time.Second
logOutputMaxBytes = 1 << 20 // 1MiB
overheadPerLog = 21 // found by testing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

We should probably also have an upper bound of buffered logs since these will take up memory, or alternatively, store logs on disk until they can be sent.

If something goes wrong with a command, there's really no telling how much it will spit out, it could be KiB's or it could be GiB's (in extreme cases). We also have limits in place in the API that limit the output to 1 MiB, although I can't recall the details if we store both the head and tail end of logs or simply cutoff at 1 MiB.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Not a bad idea. Can address in a follow on PR.

mafredri reacted with thumbs up emoji
agent/logs.go Outdated
break
}
req.Logs = append(req.Logs, log)
n++
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think this change here demonstrates a downside of using PR stacking too much. I don't think it's a good idea to split up an implementation like this, especially a new one (I might be of a different opinion for something much bigger).

Here is a change that introduces more advanced logic in a place where I left a simple suggestion in the previous PR. I think that comment applies here too (as in we don't need a clone of the slice, we can utilize the backing array/append semantics). But based on the comment from my previous PR, this may be easily overlooked due to the change in this one.

agent/logs.go Outdated
if len(log.Output) > logOutputMaxBytes {
logger.Warn(ctx, "dropping log line that exceeds our limit")
n++
continue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

How about truncating instead of dropping entirely?

@spikecurtisspikecurtisforce-pushed thespike/10534-log-batch-size branch from27b55aa toe941fbcCompareFebruary 9, 2024 11:54
@spikecurtisspikecurtis changed the base branch fromspike/10534-log-sender tospike/10534-log-sender-handle-log-limit-exceededFebruary 9, 2024 11:54
@spikecurtisspikecurtisforce-pushed thespike/10534-log-sender-handle-log-limit-exceeded branch from89ce3e7 toc8e7282CompareFebruary 9, 2024 12:05
@spikecurtisspikecurtisforce-pushed thespike/10534-log-batch-size branch frome941fbc toede6f2fCompareFebruary 9, 2024 12:05
@spikecurtisspikecurtisforce-pushed thespike/10534-log-sender-handle-log-limit-exceeded branch fromc8e7282 to0515169CompareFebruary 9, 2024 12:53
@spikecurtisspikecurtisforce-pushed thespike/10534-log-batch-size branch fromede6f2f to743f5f3CompareFebruary 9, 2024 12:53
Base automatically changed fromspike/10534-log-sender-handle-log-limit-exceeded tospike/10534-log-senderFebruary 9, 2024 12:57
@spikecurtisspikecurtis merged commit743f5f3 intospike/10534-log-senderFeb 9, 2024
@spikecurtisspikecurtis deleted the spike/10534-log-batch-size branchFebruary 9, 2024 12:57
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@mafredrimafredrimafredri left review comments

@kylecarbskylecarbskylecarbs approved these changes

Assignees

@spikecurtisspikecurtis

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@spikecurtis@mafredri@kylecarbs

[8]ページ先頭

©2009-2025 Movatter.jp