A-1591: Add OTLP job log export - #4121
Conversation
Squashed from #3979. Adds an opt-in job log OTLP sink. When --job-logs-otlp / BUILDKITE_JOB_LOGS_OTLP is enabled, the bootstrap emits job output as OpenTelemetry log records using the existing OTLP exporter environment configuration, in addition to the normal Buildkite job log stream. - Bootstrap-only architecture: single emitter with structured phase/hook metadata and native trace correlation when OTel tracing is enabled. - Child-process output and bootstrap control output (section headers, prompts, comments, warnings) are both mirrored, matching the downloadable Buildkite job log. - OTLP records are redacted with the same live secret needle set as the job log, including secrets split across writes and secrets added mid-command via the Job API. Co-authored-by: Chris Atkins <chris.a@buildkite.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
0ad8e82 to
a61626a
Compare
8e510ef to
bf02f2f
Compare
![buildsworth-bk-app[bot]](https://avatars.githubusercontent.com/in/3010600?s=60&v=4){kind=small}
There was a problem hiding this comment.
This touches the job-log export and secret-redaction path, so I'm leaving this as a comment rather than approving.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 9086, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
About buildsworth
Model: gpt-5.6-sol with xhigh thinking.
How to request a review: Comment @buildsworth-bk review on the PR, or request buildsworth-bk as a reviewer.
Risk labels (how buildsworth classifies risk) — buildsworth classifies risk itself from the diff. To let it approve, grant L2 approval by mentioning @buildsworth-bk (see L2 approval grant):
- L1 — Low risk (dep bumps, docs/copy, lockfiles, small presentational fixes). buildsworth may approve by default.
- L2 — Standard risk (new UI, additive API fields, refactors). Approved only with an L2 grant; otherwise comment-only.
- L3 — High risk (auth, migrations, payments, secrets, perf-critical paths). Human review always required.
bf02f2f to
32fcbea
Compare
| ), provider, otlpJobAttributes(e)), nil | ||
| } | ||
|
|
||
| func newOTLPJobLoggerWithLogger(log otellog.Logger, provider *sdklog.LoggerProvider, baseAttrs []otellog.KeyValue) *otlpJobLogger { |
There was a problem hiding this comment.
Why does newOTLPJobLogger take a context arg, but this function does not?
There was a problem hiding this comment.
It's because this function isn't context-aware. The function exist to reduce a bit of duplicated code. Maybe it's not worth the indirection 🤔
There was a problem hiding this comment.
It stores context.Background() into the ctx field. That could be passed in?
There was a problem hiding this comment.
True, done. I also added a comment to explain that the passed in ctx isn't about lifetime control.
DrJosh9000
left a comment
DrJosh9000
left a comment
There was a problem hiding this comment.
Happy to ship it and see how it goes
There was a problem hiding this comment.
I found no new issues in this pass. This remains on the job-log export and secret-redaction path, so it still needs human sign-off.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 9132, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
Address review feedback and simplify the correlation design on top of the squashed base: - Correlate exported log records with the current phase/hook span only: each record carries the trace ID and the span ID of the nearest enclosing phase or hook span at emit time. Per-record attributes such as buildkite.phase and buildkite.hook.* are dropped for this cut; "plugin", "checkout" and "command" are the only phases the agent defines, and inventing phase names for other stages risks exporting wrong metadata. Richer span/attribute propagation can follow in a dedicated refactor. - OTLP receives the same redacted bytes as the Buildkite job log, teed downstream of the existing stdout and logger redactors (stdoutTee and stderrTee), replacing the parallel shell output interceptor and second redaction pass. - Unhide the bootstrap --job-logs-otlp flag and align its usage text with agent start. - Extract teeWriter into its own file, remove the flushOutputRedactors wrapper.
32fcbea to
2f6464b
Compare
There was a problem hiding this comment.
I found one non-blocking edge case in OTLP body encoding below. This remains on the job-log export and secret-redaction path, so I'm leaving a comment rather than approving.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 9170, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
| record.SetObservedTimestamp(now) | ||
| record.SetSeverity(otellog.SeverityInfo) | ||
| record.SetSeverityText("INFO") | ||
| record.SetBody(otellog.StringValue(line)) |
There was a problem hiding this comment.
Non-blocking: line comes directly from arbitrary process bytes, but StringValue is serialized as an OTLP protobuf string, whose marshaling rejects invalid UTF-8. A command that emits non-UTF-8 bytes—or a valid >64 KiB line split through a multibyte rune—will therefore lose those OTLP records while the Buildkite log keeps the bytes. Could we make the body valid UTF-8 and avoid splitting valid runes before emission?
2 participants
Description
Continuation of #3979 (thanks @catkins). That branch is squashed into the first commit here; see the original PR for the full design discussion and verification notes. The second commit reworks the correlation design, per the which span owns a log line design note.
Adds an opt-in job log OTLP sink. When
--job-logs-otlp/BUILDKITE_JOB_LOGS_OTLPis enabled, the bootstrap emits job output as OpenTelemetry log records using the existing OTLP exporter environment configuration. This is an additional sink: the normal Buildkite job log stream is unchanged.What each record carries
[REDACTED]markers as the customer-facing job log.git.clone. With tracing disabled, records are emitted without correlation.We dropped the per-record
buildkite.phaseandbuildkite.hook.*attributes from the earlier iteration. The agent only definesplugin,checkoutandcommandas phases, and inventing phase names for other stages risks exporting wrong metadata. Richer attribute propagation can follow in a dedicated change.How it differs from the original PR
OutputInterceptorplumbing is removed entirely.defer e.otlpLogSpan(ctx)()right after starting a phase or hook span).Also addresses review feedback from #3979: the
job-logs-otlpbootstrap flag is no longer hidden (comment), andteeWriterlives in its own file (comment).Testing
go test ./internal/... ./clicommand/ ./agent/andgo test -race ./internal/job/). Buildkite employees may check this if the pipeline has run automatically.go tool gofumpt -extra -w .)golangci-lint runclean on touched packages