Slog logger migration - #4258
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
8a36e7a to
4465599
Compare
Amp-Thread-ID: https://ampcode.com/threads/T-01a02e82-6565-7016-b4fc-d24b6b504ad8 Co-authored-by: Benno Moskovitz <ben.m@buildkite.com>
Amp-Thread-ID: https://ampcode.com/threads/T-01a02e82-6565-7016-b4fc-d24b6b504ad8 Co-authored-by: Benno Moskovitz <ben.m@buildkite.com>
Amp-Thread-ID: https://ampcode.com/threads/T-01a02e82-6565-7016-b4fc-d24b6b504ad8 Co-authored-by: Benno Moskovitz <ben.m@buildkite.com>
Amp-Thread-ID: https://ampcode.com/threads/T-01a02e82-6565-7016-b4fc-d24b6b504ad8 Co-authored-by: Benno Moskovitz <ben.m@buildkite.com>
Amp-Thread-ID: https://ampcode.com/threads/T-01a02e82-6565-7016-b4fc-d24b6b504ad8 Co-authored-by: Benno Moskovitz <ben.m@buildkite.com>
11c083b to
80b4cfe
Compare
moskyb
added
breaking
![buildsworth-bk-app[bot]](https://avatars.githubusercontent.com/in/3010600?s=60&v=4){kind=small}
There was a problem hiding this comment.
The logger migration still has a few behavior regressions around environment handling, Windows color output, debug logging, and JSON retry metadata; details are inline.
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 19990, 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. Unless repository policy already allows L2 approval, grant it by mentioning @buildsworth-bk (see approval ceiling and 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 when repository policy or a verified grant allows it; otherwise comment-only.
- L3 — High risk (auth, migrations, payments, secrets, perf-critical paths). Human review always required.
Amp-Thread-ID: https://ampcode.com/threads/T-01a02e82-6565-7016-b4fc-d24b6b504ad8 Co-authored-by: Benno Moskovitz <ben.m@buildkite.com>
Amp-Thread-ID: https://ampcode.com/threads/T-01a02e82-6565-7016-b4fc-d24b6b504ad8 Co-authored-by: Benno Moskovitz <ben.m@buildkite.com>
Previous concerns have been addressed.
There was a problem hiding this comment.
The previous logger regressions are fixed. I have one remaining question inline about the loss of per-record version metadata; the breadth of this customer-visible logging/API migration keeps this comment-only under the L1 approval ceiling.
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 20088, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
| debugI, _ := reflections.GetField(cfg, "Debug") | ||
| if debug, ok := debugI.(bool); ok && debug { | ||
| l.SetLevel(logger.DEBUG) | ||
| l := slog.New(handler) |
There was a problem hiding this comment.
Question: The old logger attached version.FullVersion() as agent_version to every debug record (this was added specifically in c8a4bf7), but the new root logger drops it. Is removing that structured support metadata intentional?
There was a problem hiding this comment.
Still open — the root logger still does not attach agent_version to debug records.
There was a problem hiding this comment.
Still open — agent_version remains absent from root debug records on the latest revision.
There was a problem hiding this comment.
The previous agent_version question remains open. The latest parser change introduces one blocking log-level regression inline.
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 20441, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
| if err != nil { | ||
| return err | ||
| var level slog.Level | ||
| if err := level.UnmarshalText([]byte(levelStr)); err != nil { |
There was a problem hiding this comment.
Blocking: slog.Level.UnmarshalText only recognizes DEBUG, INFO, WARN, and ERROR (plus numeric offsets), so the documented fatal alias now fails here. CreateLogger then falls back to INFO after warning, meaning an existing --log-level fatal or BUILDKITE_AGENT_LOG_LEVEL=fatal configuration becomes much more verbose instead of filtering at ERROR. Please preserve the fatal → slog.LevelError mapping, with coverage, before delegating to UnmarshalText.
There was a problem hiding this comment.
i've removed any documentation of the fatal log level
79cf209 to
2c8fd72
Compare
Previous concerns have been addressed.
There was a problem hiding this comment.
The fatal-alias concern is settled as an intentional breaking change, while the agent_version question remains open. I found no new issues in this revision; the customer-visible logging API migration remains beyond the L1 approval ceiling.
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 20663, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
2 participants
Description
Currently, the agent uses a custom leveled logger in the
loggerpackage. Generally, within Buildkite's various go projects, we're standardising on using the builtinslogpackage as our logger of choice.Given that we're releasing a major version soon and log format is (regrettably) an API surface, we have a good opportunity to update the agent to use more standard tooling.
This PR removes the
loggerpackage, replacing it withslogandlmittmann/tint, with a couple (but not many) tweaks and changes to the defaults.Also in this PR, I've updated many of the log messages to make better use of log attributes, rather than encoding them in prose. This makes JSON logs better.
Context
The
loggerpackage and its nonstandardness have been annoying me for a long time.Changes
This PR contains the following breaking changes:
NOTICEandFATALlog levels have been removed, with the default log level now beingINFO. AllNOTICElogs have becomeINFO, and allFATALhave becomeERROR(that then exit the application)tsis now calledtimeMany small changes in labels, colour, styling, quoting and field placement
Timestamps are now include the time zone
Output will only be coloured if stderr is a TTY (and colour hasn't been disabled)
The agent now respects
NO_COLOR[Process]and[JobRunner]becamecomponent=Processandcomponent=JobRunner; agent/hook prefixes are now regular attributes.Testing
go test ./...). Buildkite employees may check this if the pipeline has run automatically.go tool gofumpt -extra -w .)Disclosures / Credits
Amp did most of the typing: https://ampcode.com/threads/T-01a02e82-6565-7016-b4fc-d24b6b504ad8