iframe-proxy

geraldw-ai · 2026-10-20T00:33:32Z

What changed?

handler.Handle() (common/log/slog.go) now maps record.Level by range instead of exact equality:

switch {
case record.Level >= slog.LevelError:
    h.logger.Error(record.Message, tags...)
case record.Level >= slog.LevelWarn:
    h.logger.Warn(record.Message, tags...)
case record.Level >= slog.LevelInfo:
    h.logger.Info(record.Message, tags...)
default:
    h.logger.Debug(record.Message, tags...)
}

Why?

Enabled() and Handle() disagree, and the file already contains the correct mapping. Enabled() admits a record by range:

func (h *handler) Enabled(_ context.Context, level slog.Level) bool {
    ...
    return h.zapLogger.Core().Enabled(convertSlogToZapLevel(level))
}

func convertSlogToZapLevel(l slog.Level) zapcore.Level {
    switch {
    case l >= slog.LevelError: return zapcore.ErrorLevel
    case l >= slog.LevelWarn:  return zapcore.WarnLevel
    case l >= slog.LevelInfo:  return zapcore.InfoLevel
    default:                   return zapcore.DebugLevel
    }
}

Handle() then matched by exact equality against the four named constants, with an empty default: and return nil. So a record at an intermediate level passed the gate, was reported as handled, and was never written — no log, no error, no metric.

slog levels are plain ints and the API explicitly supports values between the named constants. convertSlogToZapLevel()'s own doc comment acknowledges this and links the Go design proposal's Levels section: "there is some room between slog levels while zap levels are continuous, so we can't 1:1 map them." The handler knew about the gaps; Handle() just didn't honor them.

Latent today. NewSlogLogger()'s three call sites all feed common/nexus/nexusrpc (nexus_completion_http_handler.go:117, nexus_operation_http_handler.go:76, :109), which logs only via .Error() and .Warn() — and nothing in the repo logs at a non-named level, so no record is currently lost.

Still worth fixing: NewSlogLogger() is exported API, the loss is silent (no log, no error, Handle() returns nil), and Enabled() advertises the levels Handle() discards. A trap for the next caller, not a live defect.

How did you test it?

TestSlogFromLayeredLogger passes before and after, but cannot catch this: it only ever logs at slog.LevelInfo, an exact-match case, so the empty default: is never reached.

Added TestSlogLevelMapping to the same file — table-driven over nine levels, asserting for each that Enabled() admits it and that a record then arrives at the expected zap level. Written before the fix and run against unmodified main first:

PASS  debug / info / warn / error         <- the four named levels
FAIL  below debug             (level -5)  <- "admitted by Enabled but dropped by Handle"
FAIL  between debug and info  (level -3)
FAIL  between info and warn   (level  1)
FAIL  between warn and error  (level  5)
FAIL  above error             (level  9)

All nine pass with the fix. go vet ./common/log/ is clean.

Potential risks

Low, and confined to one method. No record that was previously logged changes level — all four named levels map identically before and after, which the new test confirms: its debug/info/warn/error cases pass on unmodified main. The only change is that records previously discarded now appear, at zapcore.DebugLevel for anything below slog.LevelInfo and zapcore.ErrorLevel for anything at or above slog.LevelError, following convertSlogToZapLevel().

And since nothing in the repo emits at a non-named level, no in-repo call path changes behavior at all. The change is observable only to an external consumer of NewSlogLogger() that logs at intermediate levels, which would see those records start appearing and a corresponding rise in log volume.

0xamlab · 2026-10-20T13:16:21Z

handler.Handle() matches record.Level by exact equality against the four named slog levels, with an empty default that discards anything else and returns nil, so the caller believes the record was written. slog levels are plain ints and the API permits values between the constants, so a record at slog.LevelInfo+1 is admitted by Enabled() (which maps by range via convertSlogToZapLevel()) and then silently dropped by Handle(). Map by range in Handle() too, mirroring convertSlogToZapLevel(). Records at levels between or beyond the named constants are emitted instead of discarded; the four named levels are unaffected.

geraldw-ai · 2026-10-20T16:54:40Z

@0xamlab Thanks for your time!

abhisheksharma2411

Ran it locally and it behaves as described. I also put the old exact-match switch back with your new test in place, and it fails on all four intermediate levels — so the test is genuinely pinning the mapping, not just passing next to it.

One thing I noticed while reading, not a change request: Enabled() and Handle() still disagree in a second spot. extractZapLogger() unwraps through throttledLogger, so Enabled() asks the zap core directly while Handle() writes through h.logger, which is still the throttled one. When the limiter is out of tokens the record goes nowhere, with no log and no error — same silent shape you're fixing here, just from throttling instead of level mapping. It doesn't look reachable from the three nexus call sites today (they pass the plain logger, not the one from resource/fx.go), so same "trap for the next caller" category as the bug itself.

Small one: the logging-call-at assertion hardcodes slog_test.go:41, which is why adding an import made you edit an unrelated test. Might be worth deriving that line number so the next import doesn't do it again.

0xamlab · 2026-10-21T13:12:00Z

You're welcome — the intermediate-level cases were the gap worth pinning down. Range checks mirror convertSlogToZapLevel cleanly.

The logging-call-at assertion hardcodes the line of the Info() call it is checking, so any edit above that point - adding an import, for instance - requires updating an otherwise unrelated assertion. Derive the line with runtime.Caller(0) instead. The assertion still discriminates: breaking the derivation makes it fail, and inserting a line above the Info() call no longer does.

…-temporal into slog-custom-levels

geraldw-ai · 2026-10-21T22:51:33Z

Ran it locally and it behaves as described. I also put the old exact-match switch back with your new test in place, and it fails on all four intermediate levels — so the test is genuinely pinning the mapping, not just passing next to it.

One thing I noticed while reading, not a change request: Enabled() and Handle() still disagree in a second spot. extractZapLogger() unwraps through throttledLogger, so Enabled() asks the zap core directly while Handle() writes through h.logger, which is still the throttled one. When the limiter is out of tokens the record goes nowhere, with no log and no error — same silent shape you're fixing here, just from throttling instead of level mapping. It doesn't look reachable from the three nexus call sites today (they pass the plain logger, not the one from resource/fx.go), so same "trap for the next caller" category as the bug itself.

Small one: the logging-call-at assertion hardcodes slog_test.go:41, which is why adding an import made you edit an unrelated test. Might be worth deriving that line number so the next import doesn't do it again.

Thanks — both good catches!

On the throttled logger: confirmed, and it’s the same shape. extractZapLogger() unwraps past throttledLogger to the bare *zap.Logger, so Enabled() consults a zap core that knows about level thresholds but nothing about the rate limiter sitting in the wrapper it just stripped. Handle() then writes through that wrapper — throttledLogger.Info hands the real write to rateLimit as a closure, and when limiter.Allow() is false the closure is simply never invoked. No log, no error, and Handle() returns nil.

Agreed it isn’t reachable from the nexus sites today — all three pass a plain log.Logger, and nothing feeds a throttled logger to NewSlogLogger(). Deciding what Enabled() should mean when the write path is wrapped feels like a call for the logging owners rather than something to slip into this PR, so I’ll raise it separately.

On the hardcoded line: that's a good suggestion and I've just committed a change to derive it from runtime.Caller.

geraldw-ai requested review from a team as code owners October 20, 2026 00:33

geraldw-ai force-pushed the slog-custom-levels branch from 08fcf42 to 3af0ea0 Compare October 20, 2026 14:43

Merge branch 'main' into slog-custom-levels

b024eb5

abhisheksharma2411 reviewed Oct 20, 2026

View reviewed changes

geraldw-ai added 2 commits October 21, 2026 15:39

Merge branch 'slog-custom-levels' of github.com:geraldw-ai/temporalio…

51aef73

…-temporal into slog-custom-levels

geraldw-ai added 2 commits October 21, 2026 15:51

Merge branch 'main' into slog-custom-levels

dc26271

Merge branch 'main' into slog-custom-levels

6cf2038

yycptt requested a review from bergundy October 23, 2026 23:01

Merge branch 'main' into slog-custom-levels

f12e0db

Sunbelt Computer Software

PL/B Language Development and Support

Navigation Menu

Repository navigation

Stop dropping slog records at intermediate levels - #12624

Stop dropping slog records at intermediate levels#12624
geraldw-ai wants to merge 7 commits into
temporalio:mainfrom
geraldw-ai:slog-custom-levels

geraldw-ai commented Oct 20, 2026 •

edited

Loading

Uh oh!

0xamlab commented Oct 20, 2026

Uh oh!

geraldw-ai commented Oct 20, 2026

Uh oh!

abhisheksharma2411 left a comment

Uh oh!

0xamlab commented Oct 21, 2026

Uh oh!

geraldw-ai commented Oct 21, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Sunbelt Computer Software

PL/B Language Development and Support

Conversation

geraldw-ai commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed?

Why?

How did you test it?

Potential risks

Uh oh!

0xamlab commented Oct 20, 2026

Uh oh!

geraldw-ai commented Oct 20, 2026

Uh oh!

abhisheksharma2411 left a comment

Choose a reason for hiding this comment

Uh oh!

0xamlab commented Oct 21, 2026

Uh oh!

geraldw-ai commented Oct 21, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

geraldw-ai commented Oct 20, 2026 •

edited

Loading