feat: process-memory + buildkitd OTEL telemetry for CI memory diagnos… - #572
feat: process-memory + buildkitd OTEL telemetry for CI memory diagnos…#572gilescope wants to merge 9 commits into
Conversation
…tics Signed-off-by: Giles Cope <gilescope@gmail.com>
|
| Branch | Total Count |
|---|---|
| main | 4748 |
| This PR | 4758 |
| Difference | +10 (0.21%) |
📁 Changes by file type:
| File Type | Change |
|---|---|
| Go files (.go) | ❌ +10 |
| Documentation (.md) | ➖ No change |
| Earthfiles | ➖ No change |
Keep up the great work migrating from Earthly to Earthbuild! 🚀
💡 Tips for finding more occurrences
Run locally to see detailed breakdown:
./.github/scripts/count-earthly.shNote that the goal is not to reach 0.
There is anticipated to be at least some occurences of earthly in the source code due to backwards compatibility with config files and language constructs.
![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 OpenTelemetry metrics for tracking process memory (such as Alloc, HeapAlloc, HeapSys, and Sys) and adds telemetry environment configuration for buildkitd. It also adds a Reset method to the stats stream parser to allow recovery from malformed frames, complete with unit tests. Feedback from the review suggests avoiding if statements with initializers across several files to prevent potential linting issues, and tracking already-seen keys in appendOTELResourceAttributes to avoid duplicate resource attributes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Signed-off-by: Giles Cope <gilescope@gmail.com>
Signed-off-by: Giles Cope <gilescope@gmail.com>
Signed-off-by: Giles Cope <gilescope@gmail.com>
Signed-off-by: Giles Cope <gilescope@gmail.com>
| Target = attribute.Key("earth.target") | ||
|
|
||
| // BuildkitContainerName is the name of the attribute that represents the container buildkitd runs in. | ||
| BuildkitContainerName = attribute.Key("earth.buildkit.container.name") |
| _, ok := envOpts["OTEL_METRICS_EXPORTER"] | ||
| if !ok { | ||
| if envOpts["OTEL_EXPORTER_OTLP_ENDPOINT"] != "" || envOpts["OTEL_EXPORTER_OTLP_METRICS_ENDPOINT"] != "" { | ||
| envOpts["OTEL_METRICS_EXPORTER"] = "otlp" |
There was a problem hiding this comment.
The otlp value is the default exporter type value. What is the reason for setting it as an env variable anyway?
| // key collision: the inherited value describes the process that spawned | ||
| // buildkitd, ours describes buildkitd itself. Emitting the key twice is not an | ||
| // option - collectors disagree on which duplicate survives. | ||
| func appendOTELResourceAttributes(base string, attrs map[string]string) string { |
There was a problem hiding this comment.
Technically, the encoding is not correct. https://opentelemetry.io/docs/specs/otel/resource/sdk/#specifying-resource-information-via-an-environment-variable
| return nil | ||
| } | ||
|
|
||
| func addBuildkitTelemetryEnv(envOpts map[string]string, containerName, installationName string, withDocker bool) { |
There was a problem hiding this comment.
Can a single func handle initialisation of env variables?
| func addBuildkitTelemetryEnv(envOpts map[string]string, containerName, installationName string, withDocker bool) { | |
| envOpts := func buildkitTelemetryEnv(settings Settings, containerName, installationName string) map[string]string { |
|
|
||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
🤖 [Gemini AI / Rob Pike Review]
The Go standard library provides math.MaxInt64. Redefining 1<<63 - 1 as a local constant is unnecessary noise.
if value > math.MaxInt64 {
return math.MaxInt64
}|
|
||
| return attrs | ||
| } | ||
|
|
There was a problem hiding this comment.
🤖 [Gemini AI / Rob Pike Review]
In buildkitd/env.go, parseBoolEnv(key) was created specifically to catch typos like EARTHLY_WITH_DOCKER=yes and warn the user. Here strconv.ParseBool ignores the error silently. Consider using parseBoolEnv("EARTHLY_WITH_DOCKER") for consistent error handling across the codebase.
|
|
||
| value, err := strconv.ParseBool(raw) | ||
| if err != nil { | ||
| return false, fmt.Errorf( |
There was a problem hiding this comment.
🤖 [Gemini AI / Rob Pike Review]
Listing every boolean variant supported by strconv.ParseBool makes the error message unnecessarily verbose. A simpler message is clearer:
return false, fmt.Errorf("%s is set to %q, which is not a valid boolean", key, raw)3 participants
Extracted from #442 (the buildkit upgrade) so the CI memory-diagnostics telemetry can land independently of the bump.
What
Adds OpenTelemetry instrumentation to help diagnose CI memory pressure (the OOM-driven flakiness that #442 fights), plus a stats-stream recovery fix:
internal/telemetry) — registers OTEL gauges for process memory (RSS/heap), tagged with process role/nesting/target so inner vs outer earth processes are distinguishable. Wired into the existing telemetryStartpath.buildkitd) —addBuildkitTelemetryEnvforwardsOTEL_*exporter config into the buildkitd container, setsOTEL_SERVICE_NAME=EarthBuild-buildkitd, and appends resource attributes (role, nesting, container name, installation).appendOTELResourceAttributesmerges with any caller-suppliedOTEL_RESOURCE_ATTRIBUTES, dropping malformed entries.Startnow uses itsinstallationNameparameter (previously discarded).Reset()(util/statsstreamparser) — discards a buffered partial frame and re-initialises the reader, so a desynced/malformeddocker statsframe (e.g. runc collector EOF emitting a partial frame) is recoverable instead of fatal.Deliberately excluded (stay in #442)
GCPolicy.KeepBytes → ReservedSpaceinbuildkitd.go— won't compile against current buildkit.hint.Wrap/printBuildkitInfoline-reflows from the bump's linter.Verification
go build ./...— green.go test ./util/statsstreamparser/— green (newResetcoverage).go test ./buildkitd/ -run Telemetry— green (×2: env forwarding + no-op without a metrics exporter).