-
Notifications
You must be signed in to change notification settings - Fork 301
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
Zerolog in Context #507
Zerolog in Context #507
Conversation
I was unable to restore #501, so this PR will replace it |
return nil, nil | ||
} | ||
|
||
estimate := 500 * len(events.logs) | ||
estimate := averageLogSizeEstimate * len(events.logs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appreciate the change here. 👍
// internal variable names and constants | ||
const ( | ||
// number of bytes expected to be needed for the average log message | ||
averageLogSizeEstimate = 400 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good use of constants
Adds the ability to ingest and harvest log events that have been structured and formatted to the proper json spec. This is meant to be used by wrapper libraries for popular logging frameworks, and should not be called by users.
Modifies the agent to support passing log data from the new zerolog plugin into the harvest pool and sending that data to our servers.
Creates a suite of tests to measure and benchmark the performance of features of the Go agent at scale.
| ------- | --------- | | ||
| Forwarding | :heavy_check_mark: | | ||
| Metrics | :heavy_check_mark: | | ||
| Enrichment | :x: | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see span.id and trace.id being put on the log lines -- is there more enrichment that needs to be supported? Also, there are the three common attributes of entity GUID, entity name, and hostname.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By enrichment, I mean that we do not support enriching the local logs that get printed to console in this release. We add all of the enrichments to the logs, including entity GUID, entity name, and hostname at harvest
|
||
// MarshalJSON is used for testing. | ||
func (e *logEvent) MarshalJSON() ([]byte, error) { | ||
buf := bytes.NewBuffer(make([]byte, 0, averageLogSizeEstimate)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does hinting the capacity with that 400 averageLogSizeEstimate really make that much of a difference? Cool that we're thinking this hard about performance, though. I'd love to hear about your grouping of int
s that I read earlier, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! The bytes buffer gets allocated on the heap, and when it grows, it has to estimate/request more memory. Each time it does that, we incur a cpu cost. By estimating a little more closely to what we think we need, we can cut down the number of buffer resizes we need, and save us cpu time, and run time!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The struct field one blew my mind when I first read it: https://wagslane.dev/posts/go-struct-ordering/
TLDR, go allocates contiguous memory for structs in the order you specify the fields in. If the fields are mismatched sizes, it will just allocate the same amount of memory for everything. If you have big structs, this will add up very fast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's similar to C in that respect.
buf.WriteString(`"entity.guid":`) | ||
jsonx.AppendString(buf, events.entityGUID) | ||
buf.WriteByte(',') | ||
buf.WriteString(`"entity.name":`) | ||
jsonx.AppendString(buf, events.entityName) | ||
buf.WriteByte(',') | ||
buf.WriteString(`"hostname":`) | ||
jsonx.AppendString(buf, events.hostname) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's great that we can take advantage of the "common" section of the logging payload format in the context of Go apps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the latest commits, this looks great. Lots of good, quality work here to keep performance and memory utilization optimized.
// events. | ||
func (events *logEvents) split() (*logEvents, *logEvents) { | ||
// numSeen is conserved: e1.numSeen + e2.numSeen == events.numSeen. | ||
sc1, sc2 := splitSeverityCount(events.severityCount) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! This was a very large spec with lots of corners, and this code implements that with an eye to memory and cpu optimization to boot. The last-minute pivot in the interface was the right decision, and now the sample apps are simpler. Works in the UI!
Logging for the Go agent is here. This feature adds: