From 93a9a243830b3ce3ad69bc8f589c7cbda9b83c54 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 8 May 2026 04:52:24 +0000 Subject: [PATCH 1/2] docs: add LOGGING.md describing the logging philosophy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Captures the design decisions arrived at across PRs #105, #110, #113, and #114 (issues #85, #99, #111, #112) — the four-PR convergence on: - Logging is opt-in; no constructor demands a slog.Handler. - nil handler means "inherit from slog.Default()" at every layer. - Exactly one fallback path: helpers.SetupLogger. - Functional options are silent on nil (Go idiom). - Group naming follows (engine, struct) shape. Includes a prior-art section showing how each choice tracks stdlib slog, slog.DiscardHandler, OpenTelemetry otelslog, go-logr/logr, the Cheney/Pike functional-options lineage, and database/sql. Also covers the explicitly-rejected alternatives (stdout fallback, nil-warning at construction, panic on nil at option layer, mandatory handler argument, library-defined logger interface) with the failure mode for each. Architectural map shows every nil-handler path funnels through helpers.SetupLogger; history table maps the four issues to the four PRs that shipped them. This is a contributor-facing engineering document. The README's "Capturing diagnostic logs" section is unchanged for now and remains the user-facing entry point; can be linked in a follow-up if desired. --- docs/LOGGING.md | 238 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 238 insertions(+) create mode 100644 docs/LOGGING.md diff --git a/docs/LOGGING.md b/docs/LOGGING.md new file mode 100644 index 0000000..d3c0711 --- /dev/null +++ b/docs/LOGGING.md @@ -0,0 +1,238 @@ +# Logging philosophy + +This document explains how `go-polyscript` handles diagnostic logging — what +the library guarantees, what hosts can rely on, and why those decisions were +made. It also lays the choices alongside what other modern Go libraries do, +so you can see they're not idiosyncratic. + +The short version: **a host that does nothing gets diagnostic logs through +`slog.Default()`. A host that wires its own `slog.Handler` gets that handler +honored at every layer. The library never writes to `os.Stdout`/`os.Stderr` +on its own.** + +--- + +## Principles + +### 1. Logging is opt-in, never required + +No constructor demands a `slog.Handler`. Every public option that takes one +also accepts nil: + +- `polyscript.WithLogHandler[E](nil)` — top-level, generic +- `engines/{risor,starlark,extism}.WithLogHandler(nil)` — per-engine +- `engines/{risor,starlark,extism}/compiler.WithLogHandler(nil)` — compiler-level +- `engines/{risor,starlark,extism}/compiler.WithLogger(nil)` — compiler-level + +Calling any of them with nil is **a true no-op** — semantically equivalent to +not calling the option at all. It does not error, does not panic, and does +not clear state set by a prior call. A host that supplies no handler is +treated identically to a host that supplies `nil`. + +### 2. nil means "inherit from `slog.Default()`" + +Every internal construction site routes through one canonical helper: + +```go +// internal/helpers/logger.go +func SetupLogger(handler slog.Handler, engineName, groupName string) (slog.Handler, *slog.Logger) +``` + +When `handler` is nil, `SetupLogger` calls `slog.Default().Handler()` and +wraps it with `engineName` as a `slog.Group`, so a record from the Risor +evaluator looks like `risor.Evaluator.<…>` regardless of whether the host +configured a handler. When `handler` is non-nil it's returned unchanged — +the host configured it the way they want and the library doesn't second-guess +the grouping. + +This means a host can call `slog.SetDefault(...)` once at startup and have +every engine inherit it — no per-engine wiring, no per-call argument passing. + +### 3. There is exactly one fallback path + +Every optional-handler path in the package routes through `helpers.SetupLogger`. +There are no `slog.NewTextHandler(os.Stdout, …)` fallbacks. There are no +`if handler == nil { handler = … }` branches scattered through constructors. +There is no "if you forgot to wire a handler we'll print to stderr to be +helpful" behavior. + +Two consequences: + +- A misconfigured host never has the library writing to stdout/stderr behind + its back, polluting structured-log pipelines. +- Changing the default-handler behavior is a one-line change in `SetupLogger`, + not a migration across every engine constructor. + +The four loader-internal `slog.Default().Debug(...)` cleanup-error logs in +`platform/script/loader/{fromHTTP,fromDisk}.go` are the one apparent +exception — they live in code paths that don't carry a logger. They still +inherit from `slog.Default()` (same fallback target as `SetupLogger`), so +the principle holds. + +### 4. Functional options are silent on nil + +This is a Go idiom: passing `nil` to an optional functional option is +equivalent to not calling it at all. Erroring on nil violates the contract +and forces every call site into a defensive `if x != nil { … opt(x) … }` +wrap. + +We held the wrong contract for a while — the per-engine +`compiler.WithLogHandler(nil)` and `compiler.WithLogger(nil)` returned +`fmt.Errorf("log handler cannot be nil")`. That decision propagated outward: +six call sites in `polyscript.go` and `engines/{risor,starlark,extism}/new.go` +gained a `cfg.handler != nil` guard solely to avoid tripping the error +during normal construction. PR #113 (issue #111) fixed the option layer; PR +#114 (issue #112) removed the now-redundant guards. + +### 5. Group names follow `(engine, struct)` shape + +Every `SetupLogger` call uses `(engineName, structName)` form: + +```go +helpers.SetupLogger(handler, "extism", "Compiler") +helpers.SetupLogger(handler, "extism", "Evaluator") +helpers.SetupLogger(handler, "extism", "execResult") +helpers.SetupLogger(handler, "script", "ExecutableUnit") +``` + +This produces records prefixed `..=…`, which is what +a host querying their logs expects: filter by `engine=extism` to see the +WASM path; filter by `script=ExecutableUnit` to see the loader/compile +machinery; etc. + +--- + +## How this aligns with prior art + +The choices above aren't novel — they line up with conventions that are by +now mainstream in modern Go. + +### Standard library `log/slog` + +`slog.NewTextHandler(w, nil)` accepts a nil `*HandlerOptions` and falls back +to defaults. The pattern is: **constructors accept nil with sensible +defaults; only fully-built logger objects panic on nil** (e.g. `slog.New(nil)` +panics, but that's an already-built thing — different layer). + +`go-polyscript` mirrors this at the option layer: passing nil never panics, +never errors, falls back to a sensible default. + +`slog.Default()` is the canonical hook for "let the host configure logging +once, library code inherits." We use it as our nil-handler target for +exactly that reason. + +### `log/slog` discard handler + +Go 1.24 added `slog.DiscardHandler` (issue golang/go#62005) so libraries +have a clean "log nothing" sink without instantiating a handler against +`io.Discard`. We don't use it as a default — `slog.Default()` is more +useful, since the host can swap it. But the same philosophy applies: +**nil-checks in business logic are a smell; have a sentinel sink instead.** + +### OpenTelemetry `otelslog` + +`otelslog.WithLoggerProvider` defaults to the global `LoggerProvider` when +the option is omitted. Absence == default. Same shape as our +`WithLogHandler`: omit it, and the global default takes over. + +### `go-logr/logr` + +The `logr` ecosystem documents `logr.Discard()` so callers never need to +nil-check a logger field. The principle: **nil logger should not be a +foot-gun.** Pre-#85 this library had nil-handlers writing to `os.Stdout`; +post-#85/#99 nil routes through `slog.Default()`. Different mechanism, same +intent. + +### Functional options pattern (Cheney/Pike lineage) + +Dave Cheney's foundational write-ups on functional options consistently +treat the option as **optional**: calling `Server(WithTimeout(...))` opts +into a non-default value; calling `Server()` accepts the default. Erroring +on `WithTimeout(0)` would be a contract violation. Same logic applies to +our `WithLogHandler(nil)` — it's the option layer's job to accept any value +the type allows; the constructor's defaults take over when the value isn't +informative. + +### Stdlib `database/sql` + +`sql.Open(driver, dsn)` doesn't take a logger. Drivers that want to log +configure it via their own setters or rely on `log.Default()` / +`slog.Default()`. The pattern is: **the host owns the logging surface; the +library doesn't fight it.** + +We follow the same pattern with `slog.Default()` as the inheritance point. + +--- + +## What we explicitly don't do + +These are choices that look reasonable on paper but have specific failure +modes we avoided: + +- **No `os.Stdout` / `os.Stderr` fallback writes.** Pre-#99 several + evaluator constructors did `slog.NewTextHandler(os.Stdout, nil)` when + given nil. That's hostile to a host running under systemd-journal, + Docker, or anything that captures stdout for output rather than logs. The + library has no business deciding where logs go. + +- **No "Handler is nil, using default" warning at construction.** Same + evaluators emitted a one-time `Warn` log when falling back. That's noise + the host didn't ask for, especially during tests where nil is the + intentional choice. + +- **No panic on nil at the option layer.** `slog.New(nil)` panics, and + that's correct for *constructors*, but functional options aren't + constructors — they're configuration knobs, and a nil knob is "use the + default." + +- **No mandatory handler argument on `polyscript.New`.** The host's + ergonomic baseline is `polyscript.New[Risor](polyscript.FromString(src))` + — nothing else. Adding a required handler argument would force every + caller to either write `slog.Default()` explicitly (boilerplate) or pass + nil (which we'd then have to handle anyway). + +- **No "library defines its own logger interface."** `logr` predates + `slog`; we don't. There's no value in re-abstracting `slog.Handler` + behind a `Logger` interface for our use case. + +--- + +## Architectural map + +If you're reading the source, the chain is: + +``` +host code + └─ polyscript.New[E](src, polyscript.WithLogHandler[E](h)) + └─ engines/.FromXxxLoader(ldr, .WithLogHandler(h)) + ├─ engines//compiler.New(compiler.WithLogHandler(h)) + │ └─ helpers.SetupLogger(h, "", "Compiler") + └─ engines//evaluator.New(h, execUnit) + └─ helpers.SetupLogger(h, "", "Evaluator") + and execResult uses ".execResult" + +platform/script.NewExecutableUnit(h, ...) — separately: + └─ helpers.SetupLogger(h, "script", "ExecutableUnit") +``` + +Every leaf is `helpers.SetupLogger`. Every layer accepts nil. Every layer's +nil-path inherits from `slog.Default()`. + +--- + +## How we got here (history) + +Four PRs (filed against issues #85, #99, #111, #112) converged the package +on the philosophy above: + +| Issue | PR | What changed | +|---|---|---| +| #85 | #105 | Made `slog.Handler` optional in engine subpackages; introduced `helpers.SetupLogger` as the canonical fallback. | +| #99 | #110 | Replaced the last three `slog.NewTextHandler(os.Stdout, nil)` fallbacks (in `engines/*/evaluator/response.go`) with `SetupLogger`. Dropped three stale logging-related TODOs. | +| #111 | #113 | Relaxed `WithLogHandler(nil)` and `WithLogger(nil)` across the three engines from "errors on nil" to true no-op (skip set, skip clear-of-the-other-field). | +| #112 | #114 | Dropped the six `cfg.handler != nil` guards in `polyscript.go` and `engines/*/new.go` that existed solely to work around the pre-#111 nil-rejection. Tightened the top-level `WithLogHandler` godoc. | + +If you're considering adding a new construction site that takes an +`slog.Handler`, the rule is simple: **call `helpers.SetupLogger` with +`(engineName, structName)`. Don't write a nil-check. Don't construct +fallback handlers. Don't panic.** From 380e22c81b146cd05f6b73616434903a37e6a05c Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 8 May 2026 06:03:33 +0000 Subject: [PATCH 2/2] docs(LOGGING): correct three Copilot review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three substantive corrections from PR #115 review (Copilot): 1. Principle 1 over-claimed "true no-op on nil" for all four option layers. Only the compiler-level WithLogHandler/WithLogger (relaxed in #111 / PR #113) skip both the set and the clear-of-the-other- field. The top-level and per-engine WithLogHandler do plain field assignment — they accept nil but don't preserve a prior non-nil value across the same option list. Doc now distinguishes the two layers and explains why the distinction is benign for the common single-call case. 2. Principle 2 said records carry the `.` prefix "regardless of whether the host configured a handler." That's not true: helpers.SetupLogger only adds the engineName group when the handler is nil. With a host-supplied handler the prefix is just ``. Doc now describes the asymmetry and explains why it's deliberate (library-owned sink vs. host-owned sink). 3. Principle 5 used `engine=extism` / `script=ExecutableUnit` filter examples that conflate slog groups with key/value attributes. Rephrased to describe slog.Handler.WithGroup behavior accurately (group qualifier on attribute keys; JSON nesting; text prefix) and to condition the `` prefix on the nil-handler case. The fourth Copilot comment (line 233 — claimed extra leading `|` in the markdown table) was a false positive; the table source has the correct single leading pipe per row and renders as three columns on GitHub. Skipped. --- docs/LOGGING.md | 68 ++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 53 insertions(+), 15 deletions(-) diff --git a/docs/LOGGING.md b/docs/LOGGING.md index d3c0711..cf60de1 100644 --- a/docs/LOGGING.md +++ b/docs/LOGGING.md @@ -24,10 +24,24 @@ also accepts nil: - `engines/{risor,starlark,extism}/compiler.WithLogHandler(nil)` — compiler-level - `engines/{risor,starlark,extism}/compiler.WithLogger(nil)` — compiler-level -Calling any of them with nil is **a true no-op** — semantically equivalent to -not calling the option at all. It does not error, does not panic, and does -not clear state set by a prior call. A host that supplies no handler is -treated identically to a host that supplies `nil`. +A host that supplies no handler is treated identically to a host that +supplies `nil` — no error, no panic, the engine inherits from +`slog.Default()`. + +Two layers behave slightly differently when the option is called more than +once with mixed values: + +- The **compiler-level** options (`compiler.WithLogHandler`, + `compiler.WithLogger`) are **true no-ops** on nil: they skip the set + *and* the clear-of-the-other-field, so `WithLogHandler(real)` followed by + `WithLogHandler(nil)` leaves the real handler in place. This was + established in #111 / PR #113. +- The **top-level and per-engine** `WithLogHandler` options assign the + handler field directly (`c.handler = h`). They accept nil, but if a + caller passes nil after a non-nil value in the same option list, the + earlier value is replaced. In practice this isn't a concern — these + options are rarely passed twice — and the field's zero value (nil) is + the same shape, so nothing observable changes for the common case. ### 2. nil means "inherit from `slog.Default()`" @@ -39,14 +53,24 @@ func SetupLogger(handler slog.Handler, engineName, groupName string) (slog.Handl ``` When `handler` is nil, `SetupLogger` calls `slog.Default().Handler()` and -wraps it with `engineName` as a `slog.Group`, so a record from the Risor -evaluator looks like `risor.Evaluator.<…>` regardless of whether the host -configured a handler. When `handler` is non-nil it's returned unchanged — -the host configured it the way they want and the library doesn't second-guess -the grouping. +wraps it with `engineName` as a `slog.Group`. The returned logger is then +wrapped with `groupName` as a nested group. So a record from the Risor +evaluator carries the group prefix `risor.Evaluator.<…>` — the engine name +is added because the host supplied no handler. -This means a host can call `slog.SetDefault(...)` once at startup and have -every engine inherit it — no per-engine wiring, no per-call argument passing. +When `handler` is non-nil, `SetupLogger` returns it unchanged and only +wraps the *logger* with `groupName`. The host's handler keeps whatever +grouping the host configured; the library doesn't prepend its engine name. +A record from the Risor evaluator under a host-supplied handler carries +just the `Evaluator.<…>` group — the host already knows it's their handler +and presumably has their own taxonomy. + +The asymmetry is deliberate: when the library is using its own (default) +sink, prefixing with the engine name disambiguates "where did this record +come from?" When the host owns the handler, the host owns the layout. + +A host can call `slog.SetDefault(...)` once at startup and have every +engine inherit it — no per-engine wiring, no per-call argument passing. ### 3. There is exactly one fallback path @@ -95,10 +119,24 @@ helpers.SetupLogger(handler, "extism", "execResult") helpers.SetupLogger(handler, "script", "ExecutableUnit") ``` -This produces records prefixed `..=…`, which is what -a host querying their logs expects: filter by `engine=extism` to see the -WASM path; filter by `script=ExecutableUnit` to see the loader/compile -machinery; etc. +`slog.Handler.WithGroup` prepends a group qualifier to every attribute key +that follows. With a `slog.JSONHandler`, the records nest under that group +key; with a `slog.TextHandler`, attribute keys carry the group prefix +(e.g. `extism.Evaluator.scriptID=…`). + +The exact prefix depends on whether the host supplied a handler (see +principle 2): + +- **Nil handler**: prefix is `..` — `extism.Evaluator.`, + `script.ExecutableUnit.`. The engine name is the library's own + contribution; the struct name disambiguates between the compiler's logs + and the evaluator's. +- **Host-supplied handler**: prefix is `.` only — `Evaluator.`, + `ExecutableUnit.`. The host owns whatever outer grouping is in place. + +In either case the `` group is always present, so a host filtering +by group `Evaluator` reaches the per-evaluator records regardless of which +mode is active. ---