Feat: add a getLogLevel() accessor to the ADK logger - #431
Open
AmaadMartin wants to merge 3 commits into
Open
Conversation
added 3 commits
July 31, 2026 19:46
The log level was write-only: setLogLevel() had no counterpart, so no caller could answer "is DEBUG enabled?" without swapping globalThis.console and observing whether output appeared. Add an optional getLogLevel?(): LogLevel | undefined member to the public Logger interface, implement it on SimpleLogger, forward it from the logger facade, and export a module-level getLogLevel() that delegates to the active logger. The member is optional so that existing Logger implementations - including the 11 in-repo test fixtures and any external implementer of the documented setLogger() extension point - keep compiling untouched. The return type is LogLevel | undefined because a logger with no single level to report (a custom logger predating this member, or the NoOpLogger installed by setLogger(null)) must be distinguishable from one genuinely sitting at INFO.
Append a getLogLevel describe block as a sibling of the existing setLogger block, so it gets its own resetLogger hooks instead of inheriting the setLogLevel(DEBUG) beforeEach that would make the "default is INFO" assertion meaningless. Covers the default logger (initial INFO and the setLogLevel round-trip for all four levels), a custom logger that implements the accessor, one that does not, the no-op logger from setLogger(null), and the logger facade forwarder.
Addresses complexity review. The free getLogLevel() read currentLogger directly, duplicating the body of the facade forwarder ten lines below and giving the accessor two independent delegation paths. It also broke this file's own convention: the sibling free setLogLevel() delegates through the facade rather than touching currentLogger. Delegate through the facade instead, so there is one path. Behaviour is unchanged, and the facade forwarder is now load-bearing for the module-level accessor too: deleting it fails 9 tests rather than 1. Also drop the fenced usage example from the getLogLevel doc comment - the prose above it already states the "treat undefined as enabled" contract, and a 7-line fence for a one-line accessor is the longest code fence in core/src.
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.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
No public issue is associated with this change, so none is cited.
Problem:
core/src/utils/logger.tsis write-only with respect to the log level. It exportssetLogLevel(level), but there is no supported way to read the level back — it lives inSimpleLogger'sprivate logLevelfield, and the internalloggerfacade only forwardssetLogLevel. No caller (and no test) can answer "is DEBUG enabled?"; the only observable is whether output reaches the console, which forces tests to swapglobalThis.consoleand capture winston's writes just to assert on level behaviour.Solution: add a first-class, non-breaking read accessor:
Logger.getLogLevel?(): LogLevel | undefined— a new optional member on the exportedLoggerinterface.SimpleLogger.getLogLevel(): LogLevel— returns the private field, narrowing the return type.logger.getLogLevel()on the internal facade — forwards to the active logger like every other facade method, so the facade stays a completeLogger.export function getLogLevel(): LogLevel | undefined— the module-level mirror ofsetLogLevel(), delegating through theloggerfacade so both accessors share one delegation path.core/src/common.ts, whichcore/src/index.tsre-exports viaexport * from './common.js'(no second export line inindex.ts— that would be a duplicate-symbol build error).Three design points worth calling out:
?is load-bearing.Loggeris public API andsetLogger()is the documented extension point for custom logging, so a required member would break every external implementer on upgrade. It would also force edits to 11Loggerobject literals across 6 in-repo test files, violating the repo's "add a new test, don't rewrite an existing one" rule for no functional gain. With the member optional, every existingLoggerimplementation and fixture compiles with zero edits; the only test file touched is the one gaining new tests.LogLevel | undefined, notLogLevel. Every delegating implementation iscurrentLogger.getLogLevel?.(), whose type understrictisLogLevel | undefined. DeclaringLogLevelwould leave the facade unable to forward without a cast, and casting to silence the compiler is not acceptable here. Implementations that do know their level (likeSimpleLogger) narrow toLogLevel, which TypeScript accepts.undefinedrather than a concrete fallback. Falling back to e.g.LogLevel.INFOwould be indistinguishable from a real answer: a caller could not tell "default logger sitting at INFO" from "custom logger, level unknown", and a guard built on it would silently skiplogger.debug(...)calls a custom DEBUG-level logger would have emitted.undefinedputs the ambiguity in the type system wherestrictforces an explicit choice, and the JSDoc states the recommended policy (treat unknown as enabled). Mutation 2 below is the concrete evidence for this decision.NoOpLoggerdeliberately does not implementgetLogLevel: it emits nothing at any level andLogLevelhas noOFFmember, so any level it returned would claim output that never happens.setLogger(null)therefore reportsundefined— "this logger has no level to report" — which is exactly true, and gives the fallback path an in-repo implementation to exercise.Deliberately out of scope (nothing was silently dropped): the ergonomic
isLogLevelEnabled(level)helper and its adoption at the eager-JSON.stringifydebug sites are queued as a separate follow-up;dev/src/utils/logger.tsAdkLoggeris left alone because it is never installed viasetLogger(), so adding the member there would ship a field with no reader; noOFFenum member;resetLoggerand theloggerfacade stay unexported.Collision check (required before implementing):
gh pr list --repo AmaadMartin/adk-js --state open --limit 200returned 200 open PRs. Scanning the file list of every one of them, no open PR touchescore/src/utils/logger.ts. Fifteen touchcore/src/common.ts(#221, #241, #280, #301, #306, #312, #317, #318, #319, #396, #397, #398, #400, #410, #419), but none edits the./utils/logger.jsexport line — they add unrelated exports, or merelyimport {logger}as consumers. The nearest-sounding PR, #349 ("pin the test log level inside the vitest workers"), touchestests/global_setup.ts,tests/setup_log_level.ts,vitest.config.tsand a newcore/test/utils/log_level_pin_test.ts— no overlap, and no conflict in behaviour (the new tests callresetLogger()inbeforeEach, which installs a freshSimpleLoggeratINFOregardless of any ambient level pin). No stacking required.Late-arriving overlap — PR #432. After this PR was opened, a sibling PR #432 ("Refactor: deduplicate the winston logger into one level-gated implementation…") was created (02:50:11Z, vs 02:47:53Z for this one) and touches two of the same files:
core/src/utils/logger.tsandcore/src/common.ts. I re-checked it rather than assume: #432 does not addgetLogLevel—grep '^\+.*getLogLevel'over its full diff returns nothing — so there is no duplicated implementation and no competing design. The overlap is textual only, and in two specific places:core/src/common.ts: both PRs rewrite the same./utils/logger.jsexport line (this one addsgetLogLevel, Refactor: deduplicate the winston logger into one level-gated implementation with one process-wide default level #432 addsWinstonLogger/WinstonLoggerOptions).core/src/utils/logger.ts: Refactor: deduplicate the winston logger into one level-gated implementation with one process-wide default level #432 replacesSimpleLoggerwith a sharedWinstonLogger, so whichever merges second must reattach the three-linegetLogLevel()accessor to the surviving class.Both target
mainand this PR is the older, already-reviewed one, so I have deliberately not restacked onto #432 — stacking the earlier PR beneath a newer sibling would invert the dependency and make this change unreviewable on its own. Flagging it here so whoever merges second rebases; the resolution is mechanical in both spots.Testing Plan
Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.
13 new tests were appended to
core/test/utils/logger_test.tsas a new top-leveldescribe('getLogLevel')block, a sibling of the existingdescribe('setLogger'). No existing test was modified, deleted, skipped or weakened; the only edit outside the new block is extending the two import statements. Placement is deliberate: the existing block'sbeforeEachcallssetLogLevel(LogLevel.DEBUG), which would make the "default level is INFO" assertion meaningless if the new tests were nested inside it. The new block has its ownresetLogger()hooks.Cases: default logger (initial
INFO, plus thesetLogLevelround-trip for all four levels); a custom logger implementing the accessor (value read back, and read-through rather than caching); a custom logger without it (returnsundefined, and does not throw); the no-op logger fromsetLogger(null)(undefined, andINFOagain afterresetLoggerso the fallback is not sticky); and theloggerfacade forwarder.The five other suites that build
Loggerfixtures were run to prove the new interface member is non-breaking — all pass with zero edits:Coverage of new code: 100% lines, branches and functions. Measured with
--coverage.include='core/src/utils/logger.ts'; the file reports% Branch 100, and readingcoverage-final.jsondirectly confirms every new statement, branch and function is hit:SimpleLogger.getLogLevel(fn / body)getLogLevel(fn / body)getLogLevel(fn / body)?.optional-call branch, module fn (logger.getLogLevel?.())?.optional-call branch, facade (currentLogger.getLogLevel?.())The
Loggerinterface member is a type declaration, erased at compile time and not counted by coverage — no coverage-ignore pragma was added (none is added anywhere in this change).Proof the tests can fail. Every one of the 13 new tests was run against mutated source and confirmed to FAIL. Seven mutations, each reverted afterwards (
diffagainst a pristine copy confirms the file was restored byte-identically before committing):SimpleLogger.getLogLevel→return LogLevel.ERROR;returns INFO for the default logger:expected 3 to be 1;reflects setLogLevel('DEBUG'):expected 3 to be +0; also('INFO'),('WARN'),returns INFO again after resetLogger,logger facade > forwards to the active loggerreturn logger.getLogLevel?.() ?? LogLevel.INFO;custom logger that does not implement getLogLevel > returns undefined:expected 1 to be undefined;null logger > returns undefined for the no-op logger: same. This is the evidence for design point 3 — a concrete fallback is observably wrongexpected undefined to be 1,+0,1,2,3, …) pluslogger facade > forwards to the active logger. Before the review fix this mutation killed only 1 test; it now kills 9, because the facade is the single delegation pathSimpleLogger.getLogLevel→return LogLevel.INFO;(stale snapshot, ignoressetLogLevel)reflects setLogLevel('DEBUG'):expected 1 to be +0;('WARN'):expected 1 to be 2;('ERROR'):expected 1 to be 3;facade forwards:expected 1 to be 2logger.getLogLevel()in the module fn,currentLogger.getLogLevel()in the facade)does not throw:expected [Function] to not throw an error but 'TypeError: currentLogger.getLogLevel …' was thrown; plusreturns undefined,returns undefined for the no-op logger,facade reports undefined when the active logger has no level, allcurrentLogger.getLogLevel is not a functionreturn LogLevel.INFO;custom logger > returns the level reported by the custom logger:expected 1 to be 2, anddelegates on every call instead of caching a copy:expected 1 to be 3setLogLevelstops forwardingdelegates on every call instead of caching a copy:expected 1 to be 3, andfacade forwards to the active logger:expected 1 to be 2Coverage was treated as a floor, not proof: mutations 5, 6 and 7 kill tests that the line/branch numbers alone would not have distinguished.
Manual End-to-End (E2E) Tests:
No automated integration or
tests/e2efixture was added, and none is warranted: this is a read-only accessor with no I/O, no network, no process boundary and no cross-package wiring. The one cross-package surface — the@google/adkexport — is already exercised, since the unit tests importgetLogLevelfrom'@google/adk'(aliased tocore/srcbyvitest.config.ts), anddocs:checkvalidates the TypeDoc entry. Adding an npm-install-heavy integration fixture for a one-line getter would not earn its cost.It was instead verified by hand against the real built package (not the vitest source alias), on both published entry points:
The emitted declarations carry the new API (
core/dist/types/utils/logger.d.ts):getLogLevel?(): LogLevel | undefined;on the interface andexport declare function getLogLevel(): LogLevel | undefined;. This scratch snippet is intentionally not committed.Local verification on the exact pushed commit (see the CI note below):
npx vitest run --project unit:core core/test/utils/logger_test.tsLogger-fixture suites abovenpm run build --workspace corenpm run lintnpm run format:checknpm run docs:checktypedoc --emit none --treatWarningsAsErrors)npm run ts:checkOn
npm run ts:check: it does not pass on the base commit — it reports 286 errors across the repo today (which is what PRs #370, #408 and #421 are about). Rather than claim a green run, the error list was captured before and after this change (git stash/git stash pop, ANSI-stripped and sorted): the two lists are byte-identical at 286 lines, and none of them is incore/src/utils/logger.ts,core/src/common.tsorcore/test/utils/logger_test.ts. This change introduces zero new type errors.Review round 1 — complexity audit
An independent complexity review raised two
shrinkfindings (its gates for suppressions, unrelated files, placement, regressions and unjustified abstraction were all clean). Both are fixed in commitFix: route the free getLogLevel() through the logger facade:Duplicated delegation path. The free
getLogLevel()readcurrentLoggerdirectly, soreturn currentLogger.getLogLevel?.()appeared twice — once there and once in the facade forwarder ten lines below — and it broke this file's own convention, since the sibling freesetLogLevel()delegates through the facade. Fixed: the free function is nowreturn logger.getLogLevel?.();. Behaviour is identical, and the facade forwarder becomes load-bearing for both entry points — mutation 3 above went from killing 1 test to killing 9.Before applying this I checked whether routing through the facade would create a dead
?.arm, sincelogger.getLogLevelalways exists at runtime. It does not: v8 still reports the file at 100% branch coverage with no zero-hit arm anywhere (verified againstcoverage-final.json). So the simplification costs nothing.Oversized doc comment. A 16-line TSDoc block, including a 7-line fenced
tsexample, for a 3-line accessor. The fence restated in code what the prose above it already says, and it was the longest code fence incore/src. Dropped; the "treatundefinedas enabled" contract remains stated in prose.Net effect: −7 lines (the diff went from +171/−3 to +164/−3), one fewer delegation path, no test changed — the reviewer explicitly noted neither finding removes a code path, and all 21 tests still pass. The full gate set (targeted tests, the five
Logger-fixture suites, build, lint,format:check,docs:check, and thets:checkbefore/after comparison) was re-run on the revised code; every result reported above is from the post-review measurement.CI
All checks pass on the final commit
6e57c253:run-testson ubuntu-latest, macos-latest and windows-latest, plus the aggregaterun-tests,check-licenseandauto-assign.One honest note: the first attempt at the macOS and Windows jobs failed, and it was not this change. Both failed identically on two tests in
tests/integration/app_loader/app_loader_test.tswithTest timed out in 40000ms(2 failed / 2689 passed) — a wall-clock timeout in the install-heavyintegrationproject, whose fixtures runnpm installandnpm run buildper directory. That file is not in this diff, this PR adds no dependency and changes no build config, and ubuntu passed throughout. Re-running the failed jobs on the identical commit turned both green, which is what confirms it as a flake rather than a real signal. The flake is being reported separately rather than patched here, since raising a timeout in an unrelated integration suite is exactly the sort of drive-by churn that does not belong in this diff.Checklist
[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.
Additional self-review notes:
any,as any/as never/as unknown as,@ts-expect-error,@ts-ignore,eslint-disable, or coverage-ignore pragma, insrc/or in tests. Verified by grepping the diff.CHANGELOG.md, no version bumps,package.jsonandpackage-lock.jsonuntouched (no dependency added —winstonwas already acoredependency), no patch/diff artifacts or scratch scripts.adk-pythonat most — Python uses the stdlibloggingmodule, whoseLoggerhas always exposedgetEffectiveLevel()/isEnabledFor(). There is no Python source file to port and no Python test to mirror, so no parity claim is made and no reference file is cited.