fix: log expected request/response conditions at debug, not warn by artiom · Pull Request #5474 · browserless/browserless · GitHub
Skip to content

fix: log expected request/response conditions at debug, not warn#5474

Open
artiom wants to merge 1 commit into
mainfrom
fix/function-non200-response-debug
Open

fix: log expected request/response conditions at debug, not warn#5474
artiom wants to merge 1 commit into
mainfrom
fix/function-non200-response-debug

Conversation

@artiom

@artiom artiom commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Why

Two warn-level logs fire on expected, non-actionable conditions and produce a large volume of low-signal warnings.

1. /function response handler — src/shared/utils/function/handler.ts

page.on('response') logged a warn for every non-OK response — favicon 404s, redirects, and other routine sub-resource misses during a normal page load. These are expected and not actionable, and inconsistent with the sibling page.on(...) handlers (which log at trace).

Now logged by status:

status level
5xx (server error), 403 (forbidden / blocked), 429 (rate-limited) warn
everything else (404s, redirects, …) debug

Server errors and blocking/rate-limit responses are the actionable cases (target down, bot-detection) and stay visible; routine misses drop to debug. The status code is now included in the message.

2. HTTP body validation failures — src/server.ts

A request body that fails schema validation is a client error — the handler already returns 400 to the caller with the details (writeResponse(res, 400, …)). Logging it server-side at warn is noise an operator can't act on, so it's lowered to debug.

Not changed

The "… disconnected unexpectedly" warn in browsers.cdp.ts stays at warn — it's deliberate crash detection (Chrome OOM/segfault/SIGKILL), a genuine signal.

@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The page response event handler now logs non-OK sub-resource responses at debug level instead of warn, and adds comments noting that these page-load responses are expected.

Changes

Page response logging

Layer / File(s) Summary
Non-OK response logging
src/shared/utils/function/handler.ts
The page.on('response') handler changes non-OK response logging from warn to debug and adds comments describing these responses as routine page-load sub-resource events.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

🐇 I hop through logs with a quieter thump,
Debug now whispers when pages go bump.
Favicon flutters, trackers may roam,
But the bunny knows this is still page-load home.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: lowering expected request/response logging from warn to debug.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/function-non200-response-debug

Comment @coderabbitai help to get the list of available commands.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

Open in Devin Review

Comment thread src/shared/utils/function/handler.ts Outdated
@artiom artiom force-pushed the fix/function-non200-response-debug branch 2 times, most recently from 7c9ed99 to e471deb Compare June 25, 2026 14:27
@artiom artiom changed the title fix: log non-200 /function responses at debug, not warn fix: log /function response failures by status (debug routine, warn errors) Jun 25, 2026
Two warn-level logs fire on expected, non-actionable conditions and
produce a large volume of low-signal warnings:

1. /function page.on('response') logged every non-OK response at warn —
   favicon 404s, redirects, and other routine sub-resource misses during
   a normal page load (inconsistent with the trace-level page.on handlers
   beside it). Now logged by status: server errors (5xx) and blocking/
   rate-limit responses (403, 429) stay at warn — those are actionable
   (target down or bot-detection) — while routine misses drop to debug.
   The status code is now included in the message.

2. HTTP body validation failures (server.ts) are client errors: the
   handler already returns 400 to the caller with the details, so the
   server-side log is not operator-actionable. Lowered to debug.

Genuine errors/warnings (incl. the unexpected-disconnect warn in
browsers.cdp.ts) are unaffected.
@artiom artiom force-pushed the fix/function-non200-response-debug branch from e471deb to 0fffe31 Compare June 25, 2026 14:35
@artiom artiom changed the title fix: log /function response failures by status (debug routine, warn errors) fix: log expected request/response conditions at debug, not warn Jun 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant