Fix: route core logger warn and error records to stderr - #697
Open
AmaadMartin wants to merge 1 commit into
Open
Conversation
Winston's Console transport writes every level to stdout unless it is given stderrLevels, so 2>/dev/null could not suppress ADK diagnostics and a pipe on stdout swallowed them. Diagnostics now follow the POSIX stream contract, which also matches adk-python. The dev package needs the same one-line change. Fork PR #672 already makes it, so this change stays inside core to avoid a duplicate.
7 tasks
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
Closes: #issue_number
Related: #issue_number
Problem: The core ADK logger writes every level to stdout, including
warnanderror. Winston's Console transport only routes a record to stderr when the level is listed in the transport'sstderrLevelsoption, andSimpleLoggerconstructed the transport with no options. So2>/dev/nullcannot suppress ADK diagnostics, and a pipe on stdout swallows them.Solution: I pass
stderrLevels: ['warn', 'error']to the Console transport.infoanddebugstay on stdout, because the bug is about diagnostics only. This matches the POSIX stream contract andadk-python, wherelogging.basicConfiginstalls aStreamHandlerthat defaults tosys.stderr.Scope: this change covers
coreonly. Thedevpackage needs the same one-line change, and open fork PR #672 ("Fix: route AdkLogger warn and error records to stderr") already makes it indev/src/utils/logger.ts. I did not duplicate it. The two changes touch disjoint files, so this branch is based onmainrather than stacked on #672.Collision check. I listed all 594 open PRs on the fork and diffed every plausibly adjacent one. #672 covers
dev(see above). #683 moves the core winston logger intocore/src/utils/logger_node.tsand #432 deduplicates both loggers; neither addsstderrLevels, so neither fixes this bug. Both restructure the code around the line I change, so expect a small merge conflict if they land first.Intentional behaviour change. A consumer that pipes ADK stdout into a log collector stops seeing warn and error lines there and must also collect stderr. No API, type, export or message-format changes, so this is not a breaking change in the semver sense.
Verified observation, not fixed here. Every level method calls
messages.join(' '). AnErrorrenders as"Error: boom"and loses its.stack; a plain object becomes"[object Object]". This matters becausedev/src/server/adk_api_server.tspasses a caughtunknownstraight tothis.logger.error(error)in about 20 places. That is a separate defect and out of scope here.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.
I added a new
describe('SimpleLogger console transport')block tocore/test/utils/logger_test.ts. The existingdescribe('setLogger')block is untouched. Six new tests:errorandwarnreach stderr and not stdout;infoanddebugreach stdout and not stderr; the stderr record keeps its upper-casedERRORtoken; a record below the log level reaches neither stream.Winston writes to
console._stdout/console._stderrdirectly, so aconsole.errorspy never sees a record. Each test installs aConsolebuilt on two capture streams instead. That helper is local to this file. PR #672 has an equivalent helper indev/test, because there is no precedent fordev/testimporting fromcore/testand ten shared lines do not justify inventing one.Coverage of the changed lines is 100%, measured with
--coverage.include='core/src/utils/logger.ts'. The remaining uncovered lines in that file are pre-existingNoOpLoggermethods and level-gating early returns.npm run ts:checkreports 280 errors, but it reports the same 280 errors on a cleanmainin my environment, and the two lists are byte-identical. None of them is in a file I touched. The cause is thatBASE_AGENT_SIGNATURE_SYMBOLis emitted as a non-exportedunique symbolincore/dist/types, so@google/adkandcore/srcdisagree about its identity. That is pre-existing and unrelated.Proof the tests can fail. I ran the new tests against three mutations of the source line.
new winston.transports.Console()(the bug)AssertionError: expected '' to contain 'boom'stderrLevels: ['error']AssertionError: expected '' to contain 'careful'stderrLevels: ['WARN', 'ERROR']AssertionError: expected '' to contain 'boom'The third mutation matters most. Winston matches
stderrLevelsagainstinfo[LEVEL], the raw lowercase level, while the format uppercasesinfo.levelfor display. Upper-cased names therefore match nothing, and the suite catches it.Integration test:
This suite first failed for me with
Error: Hook timed out in 40000ms. ItsbeforeAllrunsnpm installin a fixture directory against a 40s budget, and a cold install exceeds that behind my proxy. The suite'safterAlldeletes the fixture'snode_modulesonly on a passing run, so runs alternate cold and warm. With the fixture warm it passes with my change applied. The failure is environmental and has no path to the logger.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
I confirmed the contrast. Before the change, run 1 printed the error and run 2 printed nothing. Use
getLogger(), not aloggerbinding:core/src/common.ts:283exportsgetLogger,setLogger,setLogLevelandLogLevel, and nologger.I also checked that the API server readiness banner stays on stdout, because
tests/integration/test_api_server.ts:49matches it there:The banner is a plain
console.logatdev/src/server/adk_api_server.ts:963, so no transport option can move it. I confirmed two further preconditions by grep: no test incore/test,dev/testortests/spies onconsole._stdout,console._stderr,process.stdout.writeorprocess.stderr.writeto observe logger output, andcore/testcontains nospyOn(console, ...)at all.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.