Repository navigation
fix: port upstream log fixes, JSON log attributes and trace context (B7 of #369) - #380
mayankpande88 wants to merge 8 commits into
Conversation
(cherry picked from commit e1cd714d00d10de91b4596835cdc8d962d83a215) Conflicts: this fork imports github.com/nudgebee/logparser, which is bumped to the commit that merges coroot/logparser v1.4.2 (JSON parser, pattern rate limit). NewParser keeps this fork's SensitiveConfig argument; the rate limiter is nil until the next port. windows/ is not part of this fork.
(cherry picked from commit 18cfe3ce6740e741a10b4d5a0a791ee114474549)
(cherry picked from commit cd65ce5581bc73e989a8941e3a86c47d710349de) Conflict: this fork closes the file when Stat or Seek fails; kept.
(cherry picked from commit ab9b04971fbe16977cc224fc574c5b7622dbd3c8) Conflicts: kept this fork's logs.Init, flag style and SensitiveConfig argument. With the logparser sync, an over-limit message is still exported (under the sampled pattern); only extraction is skipped.
… bump logparser to v1.4.2 (cherry picked from commit e4be15099c51ac92116a3a231b8775d54c1a2eba) In this fork, tailing log files a process opens stays behind --enable-dynamic-log-tailing; the freopen fallback is inside that gate. The logparser bump is the nudgebee/logparser sync from the first commit.
In a storm of about 59,000 error lines in 30s, a limit of 100/s saved 139 ms of agent CPU (about 4.6 millicores), but counted 55,070 of the messages under the 'event was sampled' pattern: the storm's own pattern showed about 4,000. Pattern extraction is cheap here, and log pattern spikes are what incident detection looks at. Keep the flag for installs that need the cap.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several enhancements to log parsing and tailing. Key changes include extracting trace and span IDs from log attributes to associate them with OpenTelemetry log records, implementing a rate limiter for log pattern extraction, and allowing the tail reader to start even if the target log file does not exist yet by polling until it is created. Additionally, it adds a flag to disable JSON log parsing and introduces a mechanism to find log files held by a process when a file descriptor is redirected. Feedback on these changes highlights two issues: first, the rate limiter burst size can evaluate to zero and reject all events if the extraction limit is set below 0.1; second, findLogFiles may return duplicate log paths if a process has multiple file descriptors open for the same file, which can be resolved by deduplicating the results.
| func PatternExtractionRateLimiter() *rate.Limiter { | ||
| limit := *flags.LogPatternExtractionLimit | ||
| if limit <= 0 { | ||
| return nil | ||
| } | ||
| return rate.NewLimiter(rate.Limit(limit), int(limit*10)) | ||
| } |
There was a problem hiding this comment.
If LogPatternExtractionLimit is configured to a value less than 0.1 (for example, 0.05 to extract one pattern every 20 seconds), int(limit*10) evaluates to 0. In golang.org/x/time/rate, a limiter with a burst of 0 will reject all events, completely disabling pattern extraction.
To prevent this, ensure the burst size is at least 1 when a limit is specified.
func PatternExtractionRateLimiter() *rate.Limiter {
limit := *flags.LogPatternExtractionLimit
if limit <= 0 {
return nil
}
burst := int(limit * 10)
if burst < 1 {
burst = 1
}
return rate.NewLimiter(rate.Limit(limit), burst)
}There was a problem hiding this comment.
Fixed in e345645. The burst is now at least 1, so a limit below 0.1/s no longer rejects every event.
| func findLogFiles(pid uint32) []string { | ||
| fds, err := proc.ReadFds(pid) | ||
| if err != nil { | ||
| return nil | ||
| } | ||
| var res []string | ||
| for _, fd := range fds { | ||
| if !strings.HasPrefix(fd.Dest, "/var/log/") { | ||
| continue | ||
| } | ||
| if _, logPath := resolveFd(proc.GetFdInfo(pid, fd.Fd)); logPath != "" { | ||
| res = append(res, logPath) | ||
| } | ||
| } | ||
| return res | ||
| } |
There was a problem hiding this comment.
If a process has multiple file descriptors open for the same log file, findLogFiles can return duplicate log paths. We can deduplicate the resolved log paths using a simple map to avoid returning duplicate entries.
| func findLogFiles(pid uint32) []string { | |
| fds, err := proc.ReadFds(pid) | |
| if err != nil { | |
| return nil | |
| } | |
| var res []string | |
| for _, fd := range fds { | |
| if !strings.HasPrefix(fd.Dest, "/var/log/") { | |
| continue | |
| } | |
| if _, logPath := resolveFd(proc.GetFdInfo(pid, fd.Fd)); logPath != "" { | |
| res = append(res, logPath) | |
| } | |
| } | |
| return res | |
| } | |
| func findLogFiles(pid uint32) []string { | |
| fds, err := proc.ReadFds(pid) | |
| if err != nil { | |
| return nil | |
| } | |
| var res []string | |
| seen := make(map[string]bool) | |
| for _, fd := range fds { | |
| if !strings.HasPrefix(fd.Dest, "/var/log/") { | |
| continue | |
| } | |
| if _, logPath := resolveFd(proc.GetFdInfo(pid, fd.Fd)); logPath != "" { | |
| if !seen[logPath] { | |
| seen[logPath] = true | |
| res = append(res, logPath) | |
| } | |
| } | |
| } | |
| return res | |
| } |
There was a problem hiding this comment.
Not changing this. A duplicate path is harmless: runLogParser returns early when a parser already exists for that path (if c.logParsers[logPath] != nil { return }). Two fds on the same log file start one tailer either way.
…parser With a limit below 0.1/s, int(limit*10) was 0 and a zero-burst limiter rejects every event, so pattern extraction stopped entirely. Also pin nudgebee/logparser to the commit that decodes a JSON line once and accepts CRLF line endings.
2 participants
Summary
B7 of #369: the log fixes from upstream coroot-node-agent, on top of nudgebee/logparser#34, which merges coroot/logparser v1.4.2 into our logparser fork. Merge nudgebee/logparser#34 first, with a merge commit.
go.modhere pins its head commit.--disable-json-log-parsingturns this off.trace_id/span_idfield in a JSON log sets the OTLP record's trace context.--log-pattern-extraction-limit: a per-container cap on pattern extraction under log storms. Default0(off) here; upstream uses 100/s./var/logfile is already gone (freopen), the process's open log files are found from/proc/<pid>/fd.All five are by Nikolay Sivko.
What changes by default:
--logs-endpointunset), so this only affects installs that send logs.container_log_messages_totalkeeps the same pattern hashes.Engineering detail
Why the extraction limit defaults to 0:
Over the limit: the logparser sync emits over-limit messages instead of dropping them (see nudgebee/logparser#34).
Conflicts with this fork:
github.com/nudgebee/logparser(upstream importsgithub.com/coroot/logparser), so upstream'sgo.modbumps are replaced by the sync commit.NewParserkeeps this fork'sSensitiveConfig.logs.Init, flag style and the fd-closing paths inTailReaderare kept.--enable-dynamic-log-tailing, which still gates tailing of files a process opens.windows/isn't part of this fork.common/log_parser_test.gois updated for the newNewParsersignature.From review (e345645):
CI: gofmt, goimports, vet, golangci-lint,
go test(excluding/containers) and the build all pass in a Linux container with Go 1.26.5.Local e2e: I built agent binaries from this branch and from main and ran them as systemd services on a local Debian 12 VM (kernel 6.1), exporting logs over OTLP to a local OpenTelemetry collector.
{"level":"error","msg":"payment failed","order_id":...,"trace_id":...,"span_id":...}): this branch sent bodypayment failed, severityerror, attributeorder_id, and the trace and span IDs. Main sent the raw JSON line, with no trace context./var/logtailing (about 59,000 lines in 30s, same binary): with no limit, the agent used 1109 ms of CPU and counted 59,200 messages under one pattern. With a limit of 100/s, it used 970 ms and counted 59,000 messages, 55,070 of them under the sampled pattern.order_idattributes. JSON lines ending in\r\narrived with bodycrlf lineand theirnattribute.