Skip to content

chore: refresh Go toolchain, deps, logs, and benchmarks (go 1.27.1) - #4

Open
daveseddon-runpod wants to merge 1 commit into
feat/trace-validation-and-testsfrom
chore/go-1.25-refresh
Open

daveseddon-runpod wants to merge 1 commit into
feat/trace-validation-and-testsfrom
chore/go-1.25-refresh

Conversation

@daveseddon-runpod

Copy link
Copy Markdown

What

A refresh of the Go implementation: modernize the toolchain and dependencies, trim redundant per-line log metadata, refactor Init, add benchmarks, and run tests under -race.

Supersedes #2. #2 bundled this refresh with a UUID-strict rewrite of trace/trace.go that conflicted with #3's charset-allowlist validation. Per the reconciliation decision (charset allowlist is the canonical contract — secure and correct for non-UUID ids), the trace validation is landed in #3 and this PR carries the refresh only, with trace/ and py/ left identical to #3. Best reviewed commit-by-commit.

Stacked on #3 (feat/trace-validation-and-tests) — this diff is the refresh alone (one commit) on top of #3's validation. Merge #3 first; GitHub will retarget this to master automatically.

What changed

1. Toolchain & dependencies

  • go.mod: go → 1.27.1; enve v1.0.2 → v1.2.2; go mod tidy.
  • CI (.github/workflows/go.yaml): Go 1.27.1, added go vet, tests run with -race, plus a benchmark step. Test bodies modernized to current Go idioms (b.Loop(), range-over-int, WaitGroup.Go, SplitSeq).

2. Init refactor + doc fixes

  • Removed the goto FILLED pattern in Init, extracting metadataFromBuildInfo() (behavior unchanged).
  • Rewrote GO_README.md to describe the real API (Init + standard log/slog) — it previously documented Log()/DebugContext/… functions that don't exist — and fixed broken links in both READMEs.

3. Leaner per-line logs (opt-in to reuse)

Every record previously carried vcs_name, vcs_commit, vcs_tag, vcs_time and a full source file/line block — ~40% of a representative line, redundant on every record.

Removed from each line ~bytes
source block (AddSource:false) ~130
vcs_name (always "git"), vcs_tag, vcs_time ~65

Init now stamps vcs_commit only per line (it uniquely identifies the build); the other VCS fields are emitted once in a new structured "rplog initialized" startup record and join back via vcs_commit.

Optional to adopt — anyone building their own slog.Handler keeps full control, and Metadata.Fields() is unchanged (returns the complete set), so Datadog-style tag exporters keep every field. Neither runpod/host nor runpod/ai-api emits the full vcs_* set per line today, so this aligns the default with what real consumers already do. Full write-up in GO_README.md → "Downstream usage".

4. Benchmarks

Verification

  • go build ./..., go vet ./... clean.
  • go test -race -cover ./... green.
  • go mod tidy clean on Go 1.27.1 (enve.StringOr survives the bump).

🤖 Generated with Claude Code

Salvages the non-trace half of the Go refresh on top of the charset
trace-validation contract, so it no longer conflicts with or reverts that
validation:

- go 1.27.1; enve v1.0.2 -> v1.2.2 (+unit v1.0.0 indirect)
- Init refactored to drop the goto; Go docs corrected
- ~40% leaner per-line log metadata (~195 bytes/line saved)
- benchmarks (BENCHMARKS.md, re-measured on Go 1.27.1) and -race CI in
  .github/workflows/go.yaml
- GO_README rewritten; its inbound-validation note reconciled from the
  dropped UUID-strict claim to the shipped charset allowlist
- README Go-package links repointed to the repo root / GO_README.md

The trace package keeps the charset-allowlist validation (X-Trace-ID /
X-Request-ID: <=200 bytes, [A-Za-z0-9._-], else regenerate; sources capped at
64 and blanked when invalid); the UUID-strict variant is intentionally dropped.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@deanq deanq left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The toolchain/deps refresh and the Init split look clean. Re-raising one finding from #2 (closed for the #3 + #4 split) since the Init refactor moved here, detail inline.

Comment thread log.go

// Emit the full build metadata exactly once, so the fields no longer stamped
// on every line remain available in the logs.
slog.Info("rplog initialized",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(Re-raising this from #2, which was closed in favor of the #3 + #4 split. The Init refactor moved here, so the concern carries over.)

Two concerns with moving the VCS fields off every line into this one startup log:

  1. This goes through the same leveled jsonHandler whose level is FromTextOr("RUNPOD_LOG_LEVEL", LevelInfo). Any service running RUNPOD_LOG_LEVEL=WARN or ERROR (we already do this for log-volume reasons, e.g. SLS-446/447/448) will never emit this line at all. For that process, vcs_name/vcs_tag/vcs_time then appear nowhere: not per-line (removed above) and not at startup (filtered). The join-via-vcs_commit story silently breaks. TestInit does not cover a non-default RUNPOD_LOG_LEVEL, so this is not caught. Suggest logging this unconditionally (bypass the level filter, or use a level that is never suppressed).

  2. Dropping vcs_name/vcs_tag/vcs_time (and source) from every line is an unconditional, unflagged behavior change to Init's output for every caller. rplog is widely shared. Given the blast radius, worth confirming with an org-wide rplog.Init( grep before merge, and/or gating the change rather than flipping the default with no deprecation window. Anything faceting on @vcs_tag / @vcs_time per event in Datadog would regress.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants