docs: add LOGGING.md describing the logging philosophy#115
Merged
Conversation
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.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a contributor-facing engineering document (docs/LOGGING.md) that explains the repository’s logging design decisions, intended guarantees, and historical context around the recent nil-handler convergence work.
Changes:
- Introduces a five-principle description of logging behavior and option semantics.
- Documents how the approach aligns with common Go logging patterns and related libraries.
- Includes an architectural call-chain map and an issues/PR history table for the logging changes.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+27
to
+30
| 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`. |
Comment on lines
+42
to
+46
| 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. |
Comment on lines
+98
to
+101
| This produces records prefixed `<engine>.<struct>.<field>=…`, 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. |
Comment on lines
+228
to
+233
| | 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. | |
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 `<engine>.<struct>` 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 `<struct>`. 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 `<engine>` 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.
|
robbyt
added a commit
that referenced
this pull request
May 10, 2026
* docs: add CHANGELOG.md (Keep a Changelog 1.1.0) Closes #102 Repo had no CHANGELOG.md; users had to read GitHub release notes to find what changed between versions. With v1 ahead and a queue of breaking changes (#86, #87, #88, #89, #90, #91, #104), having a CHANGELOG before landing those gives downstream code reviewers and IDEs visibility into what's coming. Format follows Keep a Changelog 1.1.0 with the standard six headings (Added / Changed / Deprecated / Removed / Fixed / Security). [Unreleased] section captures the run of merged-but-not-released PRs since v0.7.0: - #103 polyscript.New[E] generic constructor (deprecates 12 FromXxx) - #105 slog.Handler optional in engine subpackages - #106 RequestToMap mutation fix - #107 HTTP loader MaxBodySize cap - #110 unify nil-handler on slog.Default(); drop stdout fallbacks - #113 WithLogHandler(nil)/WithLogger(nil) → no-op - #114 drop redundant nil-guards; tighten WithLogHandler doc - #115 docs/LOGGING.md - #117 fix bare top-level json.Number leak - #118 WithGlobals additive; drop dead URL check - #119 extism Eval test coverage Backfilled the three releases the issue called out (v0.5.0, v0.6.0, v0.7.0) from existing GitHub release notes, sorted into Keep a Changelog categories. Earlier releases (v0.0.x through v0.4.0) intentionally not backfilled per the issue scope; can be added in a follow-up if desired. Out of scope: the optional CI gate that fails PRs not touching CHANGELOG.md. Adds noise to small bug PRs and wants opt-out-label infrastructure to support it; better as a separate issue. * docs(CHANGELOG): clarify deprecated FromXxx still take positional handler Copilot review noted that "no constructor demands a handler" is misleading because the deprecated FromXxx constructors still take a positional `logHandler slog.Handler` argument (even though nil is accepted). Reword to distinguish the new generic constructor (no handler arg) from the deprecated ones (still take it, but nil is OK). --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
docs/LOGGING.mdcapturing the design decisions arrived at across the four-PR convergence (#105, #110, #113, #114 against issues #85, #99, #111, #112).The short version, from the doc:
What's in it
slog.Default(); one fallback path; options silent on nil;(engine, struct)group naming.slog,slog.DiscardHandler, OpenTelemetryotelslog,go-logr/logr, the Cheney/Pike functional-options lineage, anddatabase/sql. Each gets a paragraph showing how our choice maps to a precedent.helpers.SetupLogger.Audience
Contributor-facing engineering doc — explains why the package looks the way it does for someone reading the source or considering a change to a logging-construction site. The rule of thumb at the bottom: "call
helpers.SetupLoggerwith(engineName, structName). Don't write a nil-check. Don't construct fallback handlers. Don't panic."Out of scope
docs/LOGGING.mdin a follow-up if desired.Test plan
docs/LOGGING.mdrenders correctly on GitHub (no broken markdown)https://claude.ai/code/session_01C61VEAmjxSnX5Xhbab8NvL
Generated by Claude Code