Skip to content

Add OTLP job log export#3979

Open
catkins wants to merge 13 commits into
mainfrom
catkins/codex/otlp-job-logs
Open

Add OTLP job log export#3979
catkins wants to merge 13 commits into
mainfrom
catkins/codex/otlp-job-logs

Conversation

@catkins

@catkins catkins commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Description

Adds an opt-in job log OTLP sink. When --job-logs-otlp / BUILDKITE_JOB_LOGS_OTLP is enabled, the agent emits job output as OpenTelemetry log records using the existing OTLP exporter environment configuration. This is an additional sink: the normal Buildkite job log is still streamed to the control plane unchanged. The difference from that stream is that OTLP records carry native OTLP timestamps rather than timestamps encoded into the ANSI/OSC job-log body.

The OTLP endpoint and transport are intentionally inherited from the OpenTelemetry exporter configuration rather than adding Buildkite-specific endpoint flags. For logs, set OTEL_EXPORTER_OTLP_LOGS_ENDPOINT for a log-specific endpoint or OTEL_EXPORTER_OTLP_ENDPOINT for the generic endpoint; protocol selection follows OTEL_EXPORTER_OTLP_LOGS_PROTOCOL / OTEL_EXPORTER_OTLP_PROTOCOL.

The log records carry native OTLP timestamps plus a small set of Buildkite attributes for correlation: organization, pipeline, branch, queue, agent, build, job, current phase, and current hook scope/plugin where known. Trace correlation uses the native OTLP LogRecord trace context fields rather than duplicating trace_id / span_id as log attributes.

Parity with the Buildkite job log

The OTLP sink mirrors the same visible content customers see in the Buildkite UI / downloadable job log, so an OTLP destination is not confusingly different from Buildkite:

  • Child-process output (hook/command stdout & stderr) is mirrored via the shell output interceptor.
  • Bootstrap control output — section headers (~~~), prompts ($), comments (#) and warnings — is mirrored by teeing the redacted shell logger output into the exporter.

Both paths emit the same bytes (including ANSI colour codes) that land in the downloadable Buildkite log. The only intended difference is the timestamp transport: OTLP records use the native LogRecord timestamp instead of the \x1b_bk;t=… OSC markers Buildkite encodes into the raw log body. The human-visible content is identical in both destinations.

Control output is bootstrap narration rather than the output of a specific traced hook/command, so those records carry the base buildkite.* attributes but no per-hook span context; child-process records remain trace-correlated to their hook/command span.

Architecture: bootstrap-only

OTLP job log export lives entirely in the bootstrap process, which is the single home for this feature. This is a deliberate choice: the bootstrap is the process that actually runs the hooks and command, so it has structured phase/hook metadata and the active hook/command span directly, with no need to reconstruct them from the job-log text downstream. Keeping emission here also means a single emitter, a single buildkite.* attribute schema, and behaviour that does not vary with the (otherwise unrelated) tracing backend.

The bootstrap emits records whenever --job-logs-otlp is enabled, regardless of tracing backend:

  • When OpenTelemetry tracing is enabled, child-process records are emitted around the actual hook/command execution contexts, so each record carries the same native OTLP TraceId/SpanId as the corresponding exported hook/command span.
  • When tracing is disabled, records are still emitted but remain intentionally uncorrelated (empty trace context).

--tracing-propagate-traceparent only controls accepting the Buildkite control-plane traceparent; it is not required for local agent trace/log correlation.

flowchart TD
    subgraph agent["Agent process"]
        AS["buildkite-agent start<br/>--job-logs-otlp"]
    end

    AS -->|"BUILDKITE_JOB_LOGS_OTLP=true"| RUN

    subgraph bootstrap["Bootstrap process — single home for OTLP export"]
        RUN["Executor.Run<br/>JobLogsOTLP enabled?"] -->|"yes (any tracing backend)"| WIRE

        HC["hook / command<br/>stdout + stderr"] --> SH["shell OutputInterceptor<br/>(per hook/command, structured attrs)"]
        CTRL["bootstrap control output<br/>headers / prompts / comments / warnings"] --> SL["shell logger"]

        WIRE(("wire sinks"))
        WIRE --> SH
        WIRE --> SL

        SH --> TEE{{"tee raw output"}}
        TEE --> JR["job-log redactor<br/>replacer.Mux"]
        TEE --> OR["OTLP redactor<br/>replacer seeded with live needles"]
        JR --> JLOG["Buildkite job log stream"]
        OR --> EM["line emitter<br/>+ buildkite.* attributes"]

        SL --> LR["logger redactor"]
        LR --> LTEE{{"tee redacted control output"}}
        LTEE --> JLOG
        LTEE --> CEM["control line emitter<br/>+ base buildkite.* attributes"]

        EM --> EXP["OTLP log exporter<br/>(batch)"]
        CEM --> EXP
    end

    SPAN["active hook/command span<br/>only when OTel tracing enabled"] -. "native TraceId / SpanId" .-> EM
    EXP -->|"OTLP/gRPC or HTTP"| BE[("OTLP backend")]

    classDef redact fill:#fde2e2,stroke:#d33;
    class JR,OR,LR redact;
Loading

Both sinks share the same redaction (replacer, highlighted), so secrets are [REDACTED] in the OTLP records exactly as in the job log. Control output is teed after the logger redactor, so it is never re-redacted and never leaks pre-redaction bytes. Trace context is attached to child-process OTLP records only when OpenTelemetry tracing is enabled (dotted edge); otherwise records are emitted uncorrelated.

Redaction

OTLP records are redacted with the same secret needles as the customer-facing job log. The child-process OTLP copy is routed through its own replacer.Replacer, seeded from the job's live redactor Mux, so secret values are replaced with [REDACTED] before they reach the OTLP backend (including secrets split across multiple writes). Control output is mirrored from the post-redaction side of the shell logger redactor, so it inherits the same redaction without a second pass.

Relevant OpenTelemetry references:

Context

Slack context: https://buildkite-corp.slack.com/archives/C05R3MTRK38/p1780282420246339

This is a spike for moving high-rate job-log consumption toward agent-side OTLP log export, using the existing OpenTelemetry SDK plumbing in the agent.

Changes

  • Adds --job-logs-otlp and BUILDKITE_JOB_LOGS_OTLP to buildkite-agent start, propagated to the bootstrap subprocess.
  • Adds an OTLP job log writer in the bootstrap that line-buffers process output and emits OpenTelemetry LogRecords with native timestamps and structured Buildkite hook/command attributes.
  • Mirrors bootstrap control output (section headers, prompts, comments, warnings) into OTLP by teeing the redacted shell logger, giving the OTLP destination parity with the downloadable Buildkite log and the UI stream.
  • Activates OTLP export in the bootstrap whenever the flag is set, regardless of tracing backend; trace correlation is present only when OpenTelemetry tracing is enabled.
  • Redacts OTLP record bodies through the same live secret needle set as the job log redactor.
  • Uses the existing OpenTelemetry exporter environment configuration for endpoint/protocol selection instead of adding Buildkite-specific endpoint flags.
  • Emits trace/span correlation through native OTLP LogRecord fields, not duplicated log attributes.
  • Keeps Buildkite control-plane traceparent acceptance behind the existing --tracing-propagate-traceparent opt-in.
  • Detaches stored OTLP emit contexts from command cancellation while preserving trace context, so cleanup flushes do not inherit a canceled command context.

CLI help excerpt:

--job-logs-otlp                                 Export job logs directly as OpenTelemetry log records using the OTEL_EXPORTER_OTLP_LOGS_* / OTEL_EXPORTER_OTLP_* environment configuration (default: false) [$BUILDKITE_JOB_LOGS_OTLP]

Testing

  • Tests have run locally (with go test ./...). Buildkite employees may check this if the pipeline has run automatically.
  • Code is formatted (with go tool gofumpt -extra -w .)

Additional verification:

  • go build ./..., golangci-lint run ./internal/job/
  • go test -race ./internal/job/ ./internal/replacer/ ./internal/shell/ (one pre-existing, unrelated git failure — TestVerifyCommit/fails_when_commit_is_not_on_branch — reproduces on a clean tree).
  • Regression tests: TestOTLPJobLoggerRedactsSecrets, TestOTLPJobLoggerRedactsSecretsSplitAcrossWrites, TestOTLPJobLoggerControlWriter, TestOTLPJobLoggerControlWriterReused.

Validated end-to-end with a real build on a local Buildkite (buildkite.localhost) exporting to a local ClickStack (ClickHouse + OpenTelemetry collector). A locally-built agent ran a job with a secret env var, then the downloadable Buildkite log (via the v2 REST API) was compared against default.otel_logs / default.otel_traces for the same job id:

  • Control-line parity: the downloadable Buildkite log's section headers, prompts and comments (~~~ Preparing working directory, $ git clone …, # Creating …, ~~~ Running commands) all appear verbatim — including ANSI colour codes — in the OTLP records. The only difference is the OTLP records use native timestamps instead of the \x1b_bk;t=… OSC markers in the raw Buildkite log body.
  • Redaction: a command printing the secret produced leak=[REDACTED] in both the downloadable Buildkite log and the OTLP record bodies.
  • Correlation: child-process records correlated to the exported checkout and default command hook spans via native OTLP trace context; control-narration records were emitted with base attributes and no span context, as intended.

Affiliation (optional, external contributors)

Buildkite.

Disclosures / Credits

Codex and Amp assisted with implementation, adversarial review, local verification, and drafting this PR description.

@socket-security

socket-security Bot commented Jun 5, 2026

Copy link
Copy Markdown

@catkins catkins added the feature New user-facing feature! label Jun 5, 2026
@catkins catkins force-pushed the catkins/codex/otlp-job-logs branch 2 times, most recently from 5833518 to 60aa8fe Compare June 6, 2026 00:56
@catkins catkins marked this pull request as ready for review June 6, 2026 00:59
@catkins catkins requested review from a team as code owners June 6, 2026 00:59

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 60aa8fe29d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread agent/otlp_job_logger.go Outdated
@catkins catkins force-pushed the catkins/codex/otlp-job-logs branch from 60aa8fe to 2566837 Compare June 6, 2026 02:55

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2566837108

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread agent/otlp_job_logger.go Outdated
@catkins catkins force-pushed the catkins/codex/otlp-job-logs branch from 25d9724 to a4d79b0 Compare June 7, 2026 06:50

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a4d79b0c8d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread internal/job/otlp_job_logger.go Outdated

line := append(w.buf, data[:i]...)
line = bytes.TrimSuffix(line, []byte{'\r'})
w.emit(string(line))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Redact before emitting OTLP logs

When --job-logs-otlp is used with tracing-backend=opentelemetry, this writer is installed inside Executor.Run after setupRedactors, so w.out.Write(data) sends the bytes through the normal redactor but w.emit(string(line)) exports the original pre-redaction bytes to OTLP. Any secret masked from the visible Buildkite log via RedactedVars, secrets, or Job API redaction will still be sent in clear text to the OTLP collector; emit from the redacted stream or put the OTLP writer behind the same redactor.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@catkins this popped up with another review i was doing, any issue with it?

Comment thread internal/job/executor.go Outdated
Comment on lines +1323 to +1325
"buildkite.phase": "hook",
"buildkite.hook.name": "command",
"buildkite.hook.scope": "default",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Mark command output as command phase

For OpenTelemetry-backed job-log export, normal command output goes through this attributes map, but it is tagged as buildkite.phase=hook with buildkite.hook.name=command. Fresh evidence beyond the earlier scope-parser comment is this separate hard-coded internal exporter path, which makes ordinary command logs look like hook logs whenever BUILDKITE_JOB_LOGS_OTLP and the OpenTelemetry tracing backend are enabled; use the command phase attributes here instead.

Useful? React with 👍 / 👎.

@catkins catkins force-pushed the catkins/codex/otlp-job-logs branch 2 times, most recently from 852196b to 8851f51 Compare June 7, 2026 07:55

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8851f514f1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread internal/shell/shell.go
Comment on lines +497 to +500
var flushers []interface{ Flush() }
if c.shell.outputInterceptor != nil {
if cfg.captureStdout == nil && stdout != nil && stdout != io.Discard {
stdout = c.shell.outputInterceptor(ctx, stdout, cfg.outputAttrs)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Include shell-generated log lines in OTLP

When BUILDKITE_JOB_LOGS_OTLP runs with the OpenTelemetry tracing backend, the parent exporter is disabled in agent/job_runner.go, so this OutputInterceptor becomes the only OTLP log path; however it is installed only around Command.Run stdout/stderr here. Lines emitted directly by the bootstrap shell logger (Headerf, Printf, Warningf, prompts, checkout comments, etc.) still write to the normal shell writer and never pass through the interceptor, so visible job log lines generated by the agent are missing from OTLP in that configuration. Wrap the shell logger/stdout as well, or keep a full job-log exporter without duplicating command output.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: The main shell-logger path is covered now, but there are still a few setup/tracing warnings before loggerTee gets its OTLP secondary (for example setupRedactors warnings or an invalid tracing backend). Those lines still appear in the Buildkite log without reaching controlWriter, so if the parity guarantee includes startup warnings too, this path may need to attach earlier or document that exception.

@catkins catkins marked this pull request as draft June 11, 2026 23:16
@catkins catkins force-pushed the catkins/codex/otlp-job-logs branch from 8851f51 to 061c4c5 Compare June 13, 2026 00:31
@catkins

catkins commented Jun 13, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7d6cfdf486

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread internal/job/otlp_job_logger.go Outdated
// API-derived needles added during earlier phases).
return &otlpJobLogWriter{
out: out,
redactor: replacer.New(emitter, l.redactors.Needles(), redact.Redacted),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep OTLP redaction synchronized during commands

When a running command adds a redaction dynamically, for example via the Job API /redactions endpoint used by buildkite-agent redactor add or secret helpers, the normal job-log redactors are updated through e.redactors.Add, but this OTLP writer was seeded from a one-time Needles() snapshot when the command started. Any later output from the same process containing that newly redacted value is still emitted to the OTLP backend unredacted, diverging from the customer-facing log and leaking secrets whenever OTLP job logs are enabled.

Useful? React with 👍 / 👎.

@catkins

catkins commented Jun 13, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

Please focus on the new control-line parity change (latest commit 2d124cc3): the teeWriter in internal/job/executor.go that mirrors the redacted shell logger output into the OTLP controlWriter in internal/job/otlp_job_logger.go. Specifically: redaction correctness (no pre-redaction leak), concurrency/races between the cancellation goroutine and Close/flush, and that there is no double-emission of child-process output.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2d124cc3e2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread internal/job/executor.go Outdated
Comment on lines +96 to +98
w.mu.Lock()
secondary := w.secondary
w.mu.Unlock()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Serialize tee writes before shutting down OTLP

When cancellation/control output races with executor shutdown, Write snapshots secondary and releases the mutex before writing to that captured control writer. The deferred close path can then detach the secondary and flush/shutdown the OTLP provider while this in-flight write still has the old writer, so the cancellation line can be emitted after the final flush or dropped after shutdown, breaking OTLP parity in the cancellation path. Keep the tee lock held through the secondary write, or otherwise wait for in-flight secondary writes before jobLogger.Close() flushes/shuts down.

Useful? React with 👍 / 👎.

@catkins

catkins commented Jun 13, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

Updated since the last review with commit b7e9eb9e addressing adversarial review findings:

  • The per-command OTLP redactor now syncs with the live redactor Mux on each write (internal/job/otlp_job_logger.go), so secrets added mid-command (e.g. via the Job API) are redacted in OTLP output, not just secrets known at writer creation.
  • teeWriter.Write now holds the lock across the whole write (internal/job/executor.go), so detaching the OTLP control sink can't race with an in-flight write.

Validated end-to-end against a real build on buildkite.localhost exporting to ClickStack: control lines have parity with the downloadable Buildkite log, secrets are [REDACTED] in both destinations, and child-process records correlate to the checkout/default command hook spans.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b7e9eb9e4e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread agent/job_runner.go Outdated
Comment thread internal/shell/shell.go Outdated
@DrJosh9000

Copy link
Copy Markdown
Contributor

Hey @catkins just wondering if this is ready for review or still needed?

catkins and others added 6 commits July 13, 2026 11:55
Previously OTLP job log export was split across the process boundary:
the agent process emitted records when tracing was not OpenTelemetry,
and the bootstrap process emitted them when it was. That produced two
near-identical implementations, a duplicated buildkite.* attribute
schema, and two different sources of phase/hook truth (brittle log-text
regex parsing in the agent vs structured HookConfig in the bootstrap).
The same --job-logs-otlp flag also yielded materially different output
depending on the unrelated tracing backend.

Make the bootstrap process the single home for OTLP job log export:

- Remove the agent-side emitter (agent/otlp_job_logger.go) and its
  wiring in job_runner.go / run_job.go.
- Activate the bootstrap OTLP logger whenever --job-logs-otlp is set,
  regardless of tracing backend. Records are trace-correlated when
  OpenTelemetry tracing is enabled and uncorrelated otherwise.

The agent still propagates BUILDKITE_JOB_LOGS_OTLP to the bootstrap.

Amp-Thread-ID: https://ampcode.com/threads/T-019ebf08-2dbd-710f-ad6d-5bc1c842bb0a
Co-authored-by: Amp <amp@ampcode.com>
catkins and others added 3 commits July 13, 2026 11:56
The bootstrap OTLP job log writer wrapped the shell's stdout writer, which
is itself the secret redactor. Because the OTLP writer sat upstream of the
redactor, it emitted raw pre-redaction bytes to the OTLP backend while the
customer-facing job log was correctly redacted. Confirmed live against a
local ClickStack: a redacted env var appeared verbatim in OTLP records.

Route the OTLP copy through its own replacer.Replacer seeded with the live
needle set from the job's redactor Mux, so OTLP records carry the same
[REDACTED] markers as the job log. Streaming redaction also handles secrets
split across multiple writes.

- Add replacer.Mux.Needles() to expose the current needle set.
- Split otlpJobLogWriter into a tee that feeds a redactor whose downstream
  is a line-buffering OTLP emitter.
- Add regression tests for inline and split-across-writes secrets.

Amp-Thread-ID: https://ampcode.com/threads/T-019ebf08-2dbd-710f-ad6d-5bc1c842bb0a
Co-authored-by: Amp <amp@ampcode.com>
Tee the redacted shell logger output (section headers, prompts, comments,
warnings) into the OTLP exporter so exported records match the downloadable
Buildkite job log and the UI stream. Previously only child-process output was
mirrored, so OTLP destinations were missing the control lines customers see.

Amp-Thread-ID: https://ampcode.com/threads/T-019ebf08-2dbd-710f-ad6d-5bc1c842bb0a
Co-authored-by: Amp <amp@ampcode.com>
Address adversarial review findings:
- Sync the per-command OTLP redactor with the live redactor Mux on each write
  so secrets added mid-command (e.g. via the Job API) are redacted in OTLP
  output, not just secrets known when the command writer was created.
- Hold the teeWriter lock across the whole write so detaching the OTLP control
  sink cannot race with an in-flight write, preventing control output from
  landing on the emitter after it has been flushed and the provider shut down.

Amp-Thread-ID: https://ampcode.com/threads/T-019ebf08-2dbd-710f-ad6d-5bc1c842bb0a
Co-authored-by: Amp <amp@ampcode.com>
@catkins catkins force-pushed the catkins/codex/otlp-job-logs branch from b7e9eb9 to a0a4224 Compare July 13, 2026 02:20
@catkins catkins marked this pull request as ready for review July 14, 2026 21:41

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a0a4224486

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread internal/job/otlp_job_logger.go Outdated
for len(data) > 0 {
i := bytes.IndexByte(data, '\n')
if i < 0 {
e.buf = append(e.buf, data...)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Chunk unterminated OTLP log output

When a command writes a long stream without \n (for example progress output, minified JSON, or binary data), this branch keeps appending every write to e.buf and emits nothing until the command exits and Flush runs. Unlike the normal job log stream, a single unterminated line can therefore grow unbounded in agent memory and leave OTLP consumers with no records for a long-running step; impose a maximum chunk size or periodic flush for partial lines.

Useful? React with 👍 / 👎.

@buildsworth-bk-app buildsworth-bk-app Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found blockers on existing threads: the bootstrap flag can still be enabled from job env when the agent option is off, stdout/stderr OTLP wrapping still keeps separate redaction buffers, and the default batch processor can silently drop log records under load.

Want to dig deeper?

The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:

Download the buildsworth logs from build 5004, then answer my questions about the findings.

@DrJosh9000 DrJosh9000 self-requested a review July 14, 2026 22:50

@buildsworth-bk-app buildsworth-bk-app Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two of the previous blockers are fixed: the bootstrap env is now forced from agent config, and the log processor no longer uses the dropping batch queue. The stdout/stderr redaction concern is still open on the existing thread because the WithStringSearch path still splits the OTLP redaction state, so I'm leaving changes requested.

Want to dig deeper?

The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:

Download the buildsworth logs from build 5034, then answer my questions about the findings.

@buildsworth-bk-app buildsworth-bk-app Bot dismissed their stale review July 15, 2026 02:47

Previous concerns have been addressed.

@buildsworth-bk-app buildsworth-bk-app Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous stdout/stderr redaction blocker is fixed, but I found one remaining redaction-state split across separate command runs, so I'm leaving changes requested.

Want to dig deeper?

The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:

Download the buildsworth logs from build 5120, then answer my questions about the findings.

Comment thread internal/job/otlp_job_logger.go Outdated
@buildsworth-bk-app buildsworth-bk-app Bot dismissed their stale review July 15, 2026 02:59

Previous concerns have been addressed.

@buildsworth-bk-app buildsworth-bk-app Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found one new blocker after the previous redaction split was fixed: the OTLP redactor now preserves state across command wrappers, but it doesn't flush at the same boundaries as the customer-facing redactor, so partial matches can be delayed or attributed to the next phase instead of matching the Buildkite log.

Want to dig deeper?

The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:

Download the buildsworth logs from build 5123, then answer my questions about the findings.

Comment thread internal/job/otlp_job_logger.go
@buildsworth-bk-app buildsworth-bk-app Bot dismissed their stale review July 15, 2026 03:08

Previous concerns have been addressed.

@buildsworth-bk-app buildsworth-bk-app Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous redaction-flush blocker is fixed, and I don't see any new blockers. I left one non-blocking note on the existing control-output parity thread; because this touches the secrets/log-export path I'm leaving a comment rather than approving.

Want to dig deeper?

The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:

Download the buildsworth logs from build 5127, then answer my questions about the findings.

@DrJosh9000 DrJosh9000 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have thoughts

Comment thread internal/job/executor.go
Comment on lines +114 to +116
// The secondary sink is best-effort: failures must not affect the
// primary (customer-facing) job log output.
_, _ = w.secondary.Write(p)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the secondary sink considered best-effort? I can imagine a user totally ignoring the BK UI and instead expecting 100% of logs to always be present in their OTel system.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thats reasonable

Comment thread internal/job/executor.go
Comment on lines +209 to +230
jobLogger, err := newOTLPJobLogger(ctx, e)
if err != nil {
e.shell.Warningf("Failed to initialize OTLP job log exporter: %v", err)
} else {
e.otlpJobLogger = jobLogger
e.shell.SetOutputInterceptor(jobLogger.Wrap)
// Mirror the redacted shell logger control output (section headers,
// prompts, comments, warnings) into OTLP so the exported records
// match the downloadable Buildkite job log and the UI stream.
if e.loggerTee != nil {
e.loggerTee.setSecondary(jobLogger.controlWriter())
}
defer func() {
if e.loggerTee != nil {
e.loggerTee.setSecondary(nil)
}
if err := jobLogger.Close(); err != nil {
e.shell.Warningf("Failed to close OTLP job log exporter: %v", err)
}
e.otlpJobLogger = nil
}()
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The large else branch here makes me think this might be better as a method with an early return:

func (e *Executor) setupOTLPJobLogger(ctx context.Context) func() {
	jobLogger, err := newOTLPJobLogger(ctx, e)
	if err != nil {
		e.shell.Warningf("Failed to initialize OTLP job log exporter: %v", err)
		return func() {}
	}
	e.otlpJobLogger = jobLogger
	e.shell.SetOutputInterceptor(jobLogger.Wrap)
	// Mirror the redacted shell logger control output (section headers,
	// prompts, comments, warnings) into OTLP so the exported records
	// match the downloadable Buildkite job log and the UI stream.
	if e.loggerTee != nil {
		e.loggerTee.setSecondary(jobLogger.controlWriter())
	}
	return func() {
		if e.loggerTee != nil {
			e.loggerTee.setSecondary(nil)
		}
		if err := jobLogger.Close(); err != nil {
			e.shell.Warningf("Failed to close OTLP job log exporter: %v", err)
		}
		e.otlpJobLogger = nil
	}
}
Suggested change
jobLogger, err := newOTLPJobLogger(ctx, e)
if err != nil {
e.shell.Warningf("Failed to initialize OTLP job log exporter: %v", err)
} else {
e.otlpJobLogger = jobLogger
e.shell.SetOutputInterceptor(jobLogger.Wrap)
// Mirror the redacted shell logger control output (section headers,
// prompts, comments, warnings) into OTLP so the exported records
// match the downloadable Buildkite job log and the UI stream.
if e.loggerTee != nil {
e.loggerTee.setSecondary(jobLogger.controlWriter())
}
defer func() {
if e.loggerTee != nil {
e.loggerTee.setSecondary(nil)
}
if err := jobLogger.Close(); err != nil {
e.shell.Warningf("Failed to close OTLP job log exporter: %v", err)
}
e.otlpJobLogger = nil
}()
}
cleanup := e.setupOTLPJobLogger(ctx)
defer cleanup()

return nil
}

ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this Close method needs to satisfy any interfaces like io.Closer. So we should probably prefer to pass in a context arg over using context.Background(). Even if that arg will probably need to be detached (context.WithoutCancel).

// guarded by mu because control output may be written from multiple goroutines
// (e.g. the cancellation handler emitting a comment).
type otlpLineEmitter struct {
ctx context.Context

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does a context need to be stored in this struct? (The answer should probably be a comment here, perhaps with a suggestion for a future refactor.)

}
av, bv := reflect.ValueOf(a), reflect.ValueOf(b)
return av.Type() == bv.Type() && av.Comparable() && av.Interface() == bv.Interface()
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this sort of thing is only necessary if there is a concrete type implementing io.Writer in use that is not comparable...are there any? All calls to Wrap I can see pass a *bytes.Buffer, which is comparable (as a pointer).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New user-facing feature!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants