Repository navigation
feat(logging): Emit structured JSON logs from every binary - #1175
Conversation
internal/logging now builds its logger with go.miloapis.com/instrumentation/logging v0.6.0 and installs it for controller-runtime, klog and, through logr's slog bridge, slog and the standard log package. Every line is JSON with a UTC nanosecond `ts`, lowercase levels, and the `build_version` (from internal/metadata) and `instrumentation_version` fields. The binaries bind controller-runtime's standard zap flags. `--zap-devel` is the only console-output opt-in, so `--log-format` and `LOG_FORMAT` are gone and check-log-settings rejects them, and `--zap-devel`, in manifests. `--log-level` keeps its meaning: debug maps to zap level -4, so slog.Debug and controller-runtime's V(1) to V(4) still appear, printed as `debug`. An explicit `--zap-log-level` wins. logr's slog bridge gates warn lines on V(0), so a small sink wrapper hands them to zap, whose own level check still drops info at `--log-level=warn`. The CNI plugins write the same lines to their log file through SetupFile, and the MASQUE test provider uses it on stderr. GoBGP logged to io.Discard; it now logs through the process logger as `gobgp`, and the unused LogLevel setting is gone (the API socket already refuses SetLogLevel). TestLoggerAdopted runs the library's logger check over the module. The metrics half of the adoption check belongs to #1148 and #1149. docs/observability/logging.md lists the field changes for the release notes. Refs #1147
The pipeline sets `node`, `namespace`, `pod`, `service`, `endpoint`, `instance` and `cluster` as its own labels, so a log line carrying them collides with the pipeline's values. Object references become `resource_name` and `resource_namespace`, the library's subject labels. The rest get specific names: `nodeName`, `podNamespace`, `podRef`, `serviceName`, `serviceUID`, `serviceEndpoint`, `instanceName`, `clusterName`, and `caller` for a Looking Glass caller. No alert rule or log query in this repository reads these keys. Fixes #1147
privateip
changed the title
|
Two review passes ran on this PR before it was marked ready. CI is green on the head commit (all checks passed, including lint, unit, root unit, build, manifests and docs). First pass: pr-adversary returned merge with no findings. It ran go build, go vet and the logging, check-log-settings and gobgp tests, checked that the gobgp logger option is nil-safe, and checked that no reserved log keys remain. pr-conventions-reviewer returned hold, with a body warning, a title nit and three decisions. The title and body were rewritten; the code did not change. Decisions, as resolved by the human: merge order against #1173 does not matter (this branch merges cleanly onto that one and builds and tests on controller-runtime 0.25.2); the breaking log format ships to staging on merge, since nothing in this repo or the deployment config queries the changed fields; the lab check is a post-merge staging soak item. Second pass: pr-rereviewer confirmed both fixes, then held on one body claim that the change touches no YAML while Taskfile.yaml changed. yamlfmt -lint on Taskfile.yaml passed and the body was corrected. pr-conventions-reviewer returned merge with body-style warnings, which were applied in a final body edit. It also noted textual overlaps with open PRs #845, #598 and #597; all three already conflict with main on their own, so whichever lands second rebases. No code findings were applied; every fix was to the title or body. Auto-merge is enabled with the merge method, the only one the main ruleset allows. The ruleset requires one approving review and a code owner review, and dismisses approvals on push, so the PR waits for a human approval. |
ecv
left a comment
ecv
left a comment
There was a problem hiding this comment.
Approve.
Every binary now logs through the shared library logger, CI is green at 5a14072, and nothing still passes the removed --log-format or LOG_FORMAT, so no rollout crash-loop.
- In
datum-cloud/infraon main, no galactic-system or edge manifest setsLOG_FORMAT,--log-formator--zap-devel. The only hits areVECTOR_LOG_FORMAT(unrelated) and archived object-graph dumps understranded-edge-certificate-backups/. In this repo the only remaining references are the checker, its tests and the docs. hack/check-log-settingsstill validatesLOG_LEVELand--log-level, and now rejectsLOG_FORMAT,--log-formatand a truthy--zap-devel. Tests cover each case.- CNI plugins log to their configured file via
SetupFile, with stderr as the fallback. Nothing writes to stdout, so CNI results are not at risk. tests/edgeconformance/masque_test.go:1061matches"msg":"adopted attachment", and the library emitsmsg. Theverify-fabric-routing.shsubstring grep ("restoring committed") still matches inside a JSON line.verify-fabric-api.sh, the loadtest andlgqualifyonly drop the removed flag.- The timestamp key is
ts(RFC 3339 UTC), nottime, withbuild_versionandinstrumentation_versionon every line.TestSetupRoutesEveryLoggerThroughTheLibraryasserts this across slog, controller-runtime, klog and thelogpackage. The reserved-key renames (namespacetoresource_namespaceand so on) avoid collisions. Any log query that filters on the oldnamespaceortimekey needs updating.
Follow-ups:
--zap-encoder=consolestill works and is not rejected by the checker, so the docs claim that--zap-develis the only way to console output is not strictly true.--zap-log-levelis not validated by the checker, and a bad value stops the binary at startup.
Fixes #1147. Part of #1144.
Every Galactic binary now logs JSON lines that carry the binary's version and a UTC timestamp, including the lines from controller-runtime, client-go and the CNI plugins. The flag and environment variable that switched to text output are gone, and the standard development flag is now the only way to get console output.
galactic-routernow writes the embedded GoBGP server's lines, which it used to discard. Log keys the collection pipeline reserves, such asnodeandnamespace, are renamed.Important
The release notes must carry the field-change table from the logging docs. Any log query on the old timestamp key, uppercase levels or the renamed keys changes with this release. Nothing in this repository or infra reads them today.
Testing
yamlfmt -linton the changedTaskfile.yamlpass locally.