Repository navigation
[logger]: Add DebugEnabled and gate the routing debug entry - #219
Conversation
pseudomuto
left a comment
There was a problem hiding this comment.
Hey @rainman456, thanks so much for the PR (and the benchmarks)!
Anything that reduces unnecessary allocs on the hot path is a win. I have one suggestion on the shape here though. Rather than DebugEnabled(), what do you think about having Enabled(lvl Level) bool instead?
I'm thinking down the line, there'll be another spot where we might want InfoEnabled() and we'd go through a second interface (breaking) change. Having a single Enabled func would avoid that.
Some things to consider if we go this route:
zeroLevelmaps unrecognized values to Info, soEnabled(Level(42))quietly answers asEnabled(LevelInfo). Maybe worth a callout in the doc comment, or treating unknown levels as enabled so a bad value never hides logs?- Currently, the package mirrors each
Loggermethod as a package-level function overDefault(), so a matchinglogger.Enabled(lvl)would keep that symmetry.
Your numbers should carry over unchanged, since it's still one interface call and a comparison. What do you think?
|
Good call on
|
|
@rainman456 this LGTM 👍 Can we separate this from #218? |
Building the tags for a Debug entry allocates even when the configured level discards it, and the router builds that entry on every request. Profiling a request-shaped benchmark run attributed 25.89% of the router binary's total allocations to tag.String, and Director_Resolve measured 5 allocations per Resolve, most of them the four tags handed to Debug that an operator at the default level never sees. The routing decision itself is already counted in decisions_total. Logger gains DebugEnabled, which reports whether Debug entries are recorded, so hot-path callers can skip constructing Debug-only tags. ZeroLogger answers from its configured level, NoopLogger answers false, and TestLogger answers true since it records every level. The director gates its routing entry on it. Measured against the benchmarks from the previous commit, 10 rounds per side (benchstat, shared 2-vCPU host; allocs/op and B/op are deterministic and primary): Director_Resolve drops from 5 to 0 allocations and from ~501ns to ~83ns (-83%, p=0.000); the whole-path benchmark drops from 400 to 395 allocations (p=0.000); protoutil benchmarks and the no-proxy baseline are unchanged (all samples equal). Behavior at debug level is unchanged.
Review on temporalio#219 asked for a single Enabled(lvl Level) method so the next gate does not need another breaking interface change. The package-level Enabled over Default keeps the mirror with the other log functions, and an unrecognized level answers true so a bad value never hides logs.
e704cf0 to
22472a2
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
|
Thanks again @rainman456! |
Building the tags for a Debug entry allocates even when the configured level
discards the entry, and the router builds that entry on every request.
Profiling a request-shaped benchmark run attributed 25.89% of the router
binary's total allocations to tag.String. Director_Resolve measured 5
allocations per Resolve, and most of them were the four tags handed to Debug
that an operator at the default level never sees. The routing decision itself
is already counted in decisions_total.
Logger gains DebugEnabled, which reports whether Debug entries are recorded,
so hot path callers can skip constructing Debug-only tags. ZeroLogger answers
from its configured level, NoopLogger answers false, and TestLogger answers
true since it records every level. The director gates its routing entry on it.
Measured against the benchmarks from the previous PR, 10 rounds per side,
compared with benchstat. Allocs/op and B/op are deterministic and are the
primary evidence here.
Behavior at debug level is unchanged; the entry is still logged exactly as
before when the level allows it.
Full benchstat output
Notes on the run: shared 2 vCPU host (GOMAXPROCS=2, go1.27.1), so timing is
the weakest number here and the one we lean on least. Running both sides at
the same commit reproduces zero differences on every metric, so the deltas
above are the change, not the machine. The Mux_Switch/namespace-match timing
shift is on a path this diff cannot reach, so we treat it as layout noise.
The benchmarks ship in the previous PR, so every number here can be
reproduced anywhere.