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 1 commit into
Conversation
…tics Signed-off-by: Giles Cope <gilescope@gmail.com>
|
| Branch | Total Count |
|---|---|
| main | 5320 |
| This PR | 5325 |
| Difference | +5 (0.09%) |
📁 Changes by file type:
| File Type | Change |
|---|---|
| Go files (.go) | ❌ +5 |
| 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.
| if _, ok := envOpts["OTEL_METRICS_EXPORTER"]; !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.
Avoid using an if statement with an initializer. Declare the variable on a preceding line to prevent potential conflicts with project linting rules.
| if _, ok := envOpts["OTEL_METRICS_EXPORTER"]; !ok { | |
| if envOpts["OTEL_EXPORTER_OTLP_ENDPOINT"] != "" || envOpts["OTEL_EXPORTER_OTLP_METRICS_ENDPOINT"] != "" { | |
| envOpts["OTEL_METRICS_EXPORTER"] = "otlp" | |
| } | |
| } | |
| _, ok := envOpts["OTEL_METRICS_EXPORTER"] | |
| if !ok { | |
| if envOpts["OTEL_EXPORTER_OTLP_ENDPOINT"] != "" || envOpts["OTEL_EXPORTER_OTLP_METRICS_ENDPOINT"] != "" { | |
| envOpts["OTEL_METRICS_EXPORTER"] = "otlp" | |
| } | |
| } |
References
- In Go, avoid using an if statement with an initializer. Instead, declare the variable on a preceding line to prevent potential conflicts with project linting rules.
| func appendOTELResourceAttributes(base string, attrs map[string]string) string { | ||
| parts := make([]string, 0, len(attrs)+1) | ||
|
|
||
| for attr := range strings.SplitSeq(base, ",") { | ||
| attr = strings.TrimSpace(attr) | ||
| if attr == "" { | ||
| continue | ||
| } | ||
|
|
||
| if _, value, ok := strings.Cut(attr, "="); !ok || strings.TrimSpace(value) == "" { | ||
| continue | ||
| } | ||
|
|
||
| parts = append(parts, attr) | ||
| } | ||
|
|
||
| for key, value := range attrs { | ||
| if value == "" { | ||
| continue | ||
| } | ||
|
|
||
| parts = append(parts, key+"="+value) | ||
| } | ||
|
|
||
| return strings.Join(parts, ",") | ||
| } |
There was a problem hiding this comment.
To prevent duplicate resource attributes when keys are defined in both base and attrs, we should keep track of the keys we have already seen. Additionally, we should avoid using an if statement with an initializer on line 743 to comply with the project's Go guidelines.
func appendOTELResourceAttributes(base string, attrs map[string]string) string {
parts := make([]string, 0, len(attrs)+1)
seen := make(map[string]bool)
for attr := range strings.SplitSeq(base, ",") {
attr = strings.TrimSpace(attr)
if attr == "" {
continue
}
key, value, ok := strings.Cut(attr, "=")
if !ok || strings.TrimSpace(value) == "" {
continue
}
parts = append(parts, attr)
seen[strings.TrimSpace(key)] = true
}
for key, value := range attrs {
if value == "" || seen[key] {
continue
}
parts = append(parts, key+"="+value)
}
return strings.Join(parts, ",")
}References
- In Go, avoid using an if statement with an initializer. Instead, declare the variable on a preceding line to prevent potential conflicts with project linting rules.
| if value, ok := otelResourceAttributeFromEnv(key); ok { | ||
| attrs = append(attrs, attribute.String(key, value)) | ||
| } |
There was a problem hiding this comment.
Avoid using an if statement with an initializer. Declare the variable on a preceding line to prevent potential conflicts with project linting rules.
value, ok := otelResourceAttributeFromEnv(key)
if ok {
attrs = append(attrs, attribute.String(key, value))
}References
- In Go, avoid using an if statement with an initializer. Instead, declare the variable on a preceding line to prevent potential conflicts with project linting rules.
| if target := earthbuildTargetFromArgs(os.Args); target != "" { | ||
| attrs = append(attrs, attribute.String("earthbuild.target", target)) | ||
| } |
There was a problem hiding this comment.
Avoid using an if statement with an initializer. Declare the variable on a preceding line to prevent potential conflicts with project linting rules.
| if target := earthbuildTargetFromArgs(os.Args); target != "" { | |
| attrs = append(attrs, attribute.String("earthbuild.target", target)) | |
| } | |
| target := earthbuildTargetFromArgs(os.Args) | |
| if target != "" { | |
| attrs = append(attrs, attribute.String("earthbuild.target", target)) | |
| } |
References
- In Go, avoid using an if statement with an initializer. Instead, declare the variable on a preceding line to prevent potential conflicts with project linting rules.
| if value, _ := strconv.ParseBool(os.Getenv("EARTHLY_WITH_DOCKER")); value { | ||
| return "inner" | ||
| } |
There was a problem hiding this comment.
Avoid using an if statement with an initializer. Declare the variable on a preceding line to prevent potential conflicts with project linting rules.
| if value, _ := strconv.ParseBool(os.Getenv("EARTHLY_WITH_DOCKER")); value { | |
| return "inner" | |
| } | |
| value, _ := strconv.ParseBool(os.Getenv("EARTHLY_WITH_DOCKER")) | |
| if value { | |
| return "inner" | |
| } |
References
- In Go, avoid using an if statement with an initializer. Instead, declare the variable on a preceding line to prevent potential conflicts with project linting rules.
| "BUILDKIT_MAX_PARALLELISM": strconv.Itoa(settings.MaxParallelism), | ||
| } | ||
|
|
||
| withDocker, _ := strconv.ParseBool(os.Getenv("EARTHLY_WITH_DOCKER")) |
There was a problem hiding this comment.
Can you please add error handling? I can see in the original code that it was not handled, but we should.
2 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).