Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Purge logs after writing to prevent unbounded growth of buffer #70

Merged
merged 3 commits into from
Aug 8, 2017
Merged

Purge logs after writing to prevent unbounded growth of buffer #70

merged 3 commits into from
Aug 8, 2017

Conversation

zwass
Copy link
Contributor

@zwass zwass commented Aug 4, 2017

After attempting to write logs, purge the oldest logs until the
buffer is back under the limit. Limit is currently set at 500,000
logs per type (result and status).

Closes #58

// buffers.
func (e *Extension) writeAndPurgeLogs() {
for _, typ := range []logger.LogType{logger.LogTypeStatus, logger.LogTypeString} {
// Write logs
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth checking to see if the logs are already at the limit before writing, just to purge after?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I'm not sure it is (such as a case where writing brings the logs back down below the limit). We're looking at a pretty abnormal scenario if the limit is ever exceeded, so I don't think it matters too much which strategy we use, but this strategy seems potentially slightly less lossy while giving similar bounding guarantees.

Copy link
Contributor

@murphybytes murphybytes left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@marpaia marpaia left a comment

Choose a reason for hiding this comment

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

Based on the conversation on the PR, this looks good to me as well.

Copy link
Contributor

@groob groob left a comment

Choose a reason for hiding this comment

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

Looks good.

fmt.Println("ignoring unknown log type: ", err)
level.Info(e.Opts.Logger).Log(
"msg",
fmt.Sprintf("Ignoring unknown log type: %v", typ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of concatenating the log type into a string, you could add it as a log_type=typ key, value. That gives the ability to filter by key/value.

f := func(k uint64) bool {
return k == uint64FromByteKey(byteKeyFromUint64(k))
}
if err := quick.Check(f, nil); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! 👍

"msg",
fmt.Sprintf("Buffered logs limit (%d) exceeded. Purging %d logs.",
e.Opts.MaxBufferedLogs,
deleteCount,
Copy link
Contributor

Choose a reason for hiding this comment

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

deleteCount could also be a key/value, allowing someone to extract some metrics if they're valuable. I would write something like.

"msg", "Purging buffered logs. Max limit exceeded",
"limit", e.Opts.MaxBufferedLogs,
"purged_log_count", deleteCount,

@zwass
Copy link
Contributor Author

zwass commented Aug 8, 2017

Great comments re logging @groob. Thank you.

@zwass zwass merged commit f580ac5 into kolide:master Aug 8, 2017
@zwass zwass deleted the purge_logs branch August 8, 2017 22:57
@directionless directionless mentioned this pull request Aug 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants