Skip to content

fix: replace printStackTrace with RecordLog in ConfigUtil - #3638

Closed
lxcxjxhx wants to merge 1 commit into
alibaba:1.8from
lxcxjxhx:fix/config-util-logging
Closed

fix: replace printStackTrace with RecordLog in ConfigUtil#3638
lxcxjxhx wants to merge 1 commit into
alibaba:1.8from
lxcxjxhx:fix/config-util-logging

Conversation

@lxcxjxhx

Copy link
Copy Markdown

Problem

ConfigUtil.java uses e.printStackTrace() in 3 places for error handling, which bypasses Sentinel's logging framework (RecordLog).

Impact

  • Stack traces go directly to stderr instead of through the configured logging system
  • Cannot control log level, format, or output destination for these errors
  • Inconsistent with the rest of the codebase which uses RecordLog

Fix

Replace all 3 occurrences of e.printStackTrace() with RecordLog.warn() calls that include contextual information (file name or URL being loaded).

Closes #3637

Local environment limitations, relying on CI/CD automated testing.

@CLAassistant

CLAassistant commented Jul 30, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@bitdive-review

Copy link
Copy Markdown

BitDive Runtime Review - PR #3638

PR: #3638
Branch: fix/config-util-logging -> 1.8
Method: BitDive runtime trace comparison + direct HTTP verification + targeted code inspection
Verdict: APPROVE WITH NOTES
Confidence: Medium — stable flows proven with zero contract drift; changed catch blocks not runtime-traceable

Summary

This PR replaces 3 e.printStackTrace() calls with RecordLog.warn() in ConfigUtil.java (sentinel-core). All three call sites are in private static methods invoked during JVM static initialization — no control-flow, return-value, or persistence changes. Two runtime trace pairs (/version and /auth/login) show zero contract divergence between base and head, confirming the new RecordLog import does not trigger a circular class-loading deadlock. ConfigUtilTest passes 1/1 on both branches.

The actual changed catch blocks are structurally untestable in this environment: they are private static methods not reachable from any HTTP endpoint, and the unit test does not inject failures. A code-level finding worth noting: RecordLog.warn(format, param, exception) resolves to the varargs overload, and BaseJulLogger silently discards the exception stack trace — partially undermining the PR's goal of improved error diagnostics. This is a pre-existing limitation, not introduced by this PR.

Runtime Verification Matrix

Area Result Behavior Δ Meaning Evidence
Version canary (GET /version) Stable HTTP 200→200 · zero contract drift · JVM startup intact · no circular dependency New RecordLog import doesn't break class loading before / after
Login stability (POST /auth/login) Stable HTTP 200→200 · zero contract drift · 1-node trees · identical auth response Auth flow unaffected by logging change before / after
ConfigUtil unit test Stable 1/1 pass on both branches · happy-path config loading only · catch blocks not exercised Compilation + import compatibility proven code inspection
RecordLog overload resolution Note warn(fmt, param, e) → varargs overload · BaseJulLogger discards getThrowable() · stack trace silently lost Pre-existing limitation partially undermines stated goal code inspection
How to read BitDive evidence

Behavior Δ in the matrix and the red/green diff blocks inside each Change summarize the contract change.
Evidence links open the full BitDive share: call tree · SQL · writes · downstream · first divergence.

  • Trace pair — full call trees compared before/after
  • HTTP only — status/body only, no JVM tree
  • Code only — inspection of source diff (no runtime trace)

GitHub strips target="_blank"; share links open in the same tab.

Review scope and validation coverage
sentinel-dashboard
  -> VersionController.apiGetVersion()           [deploy canary — stable]
  -> AuthController.login()                       [stable flow]

sentinel-core
  -> ConfigUtil.loadPropertiesFromAbsoluteFile()  [changed — code only]
  -> ConfigUtil.loadPropertiesFromClasspathFile() [changed — code only]
  -> RecordLog.warn()                             [overload resolution analysis]
Behavior Scenario Evidence Status
JVM startup with RecordLog import GET /version Trace pair Stable — no circular dependency
Login flow unchanged POST /auth/login Trace pair Stable — zero drift
ConfigUtil compilation + happy path ConfigUtilTest Code only Stable — 1/1 pass
RecordLog overload / stack trace loss warn(fmt, param, e) resolution Code only Pre-existing limitation noted

Not covered at runtime: The 3 changed catch blocks are private static methods called during JVM static initialization, not reachable from any HTTP endpoint. The unit test does not inject failures, so the RecordLog.warn error-path code is never entered during testing.

Change #1 - ConfigUtil logging migration (stable at runtime)

Replaces 3 e.printStackTrace() calls with RecordLog.warn() in ConfigUtil's private static config-loading methods. No control-flow, return-value, or persistence changes.

Behavior contract

# ConfigUtil logging migration — runtime contract (both traced endpoints)
  HTTP 200 → 200 (identical response bodies)
  SQL: none on either path
  writes: none
  downstream REST: none
  steps: 1 → 1 (single-node execution trees)
# first divergence: NONE — CLEAN baseline on both trace pairs
# circular dependency risk: NOT REPRODUCED — head JVM starts and serves normally

Scenario matrix

Scenario Before PR After PR Purpose
Version canary 200, 0.14ms, 1 node 200, 0.24ms, 1 node Prove JVM startup with RecordLog import
Login stability 200, 0.52ms, 1 node 200, 0.39ms, 1 node Prove auth flow unaffected

Trace evidence

Trace Scenario HTTP SQL / REST Result
before version 200 none Clean baseline
after version 200 none Zero drift
before login 200 none Clean baseline
after login 200 none Zero drift

Contract delta

Layer Before PR After PR Result
HTTP 200 on both endpoints 200 on both endpoints stable
Persistence none none stable
Downstream none none stable
Execution tree 1 node each 1 node each zero drift
JVM startup clean clean (RecordLog import loads) stable
Change #2 - RecordLog overload resolution (stack trace loss)

Code analysis finding: the PR's stated goal is improved error diagnostics, but RecordLog.warn(format, param, exception) resolves to the varargs overload warn(String format, Object... args), not warn(String msg, Throwable e). BaseJulLogger.log() calls only FormattingTuple.getMessage() and discards getThrowable() — the exception message is logged but the stack trace is silently lost.

This is a pre-existing limitation affecting the entire Sentinel codebase, not introduced by this PR. The PR still improves the situation by routing messages through the configurable logging framework (level, format, destination) instead of raw stderr.

Behavior contract

# RecordLog.warn overload resolution — code analysis
- e.printStackTrace()                           // full stack trace to stderr
+ RecordLog.warn("msg {}", param, exception)     // resolves to warn(String, Object...) varargs
                                                 // BaseJulLogger.log() discards getThrowable()
                                                 // → message logged, stack trace SILENTLY LOST
# pre-existing limitation — not introduced by this PR
# workaround: use 2-arg overload RecordLog.warn("message", e) to preserve stack trace

Contract delta

Layer Before PR After PR Result
Error output destination raw stderr (System.err) logging framework (configurable level/format) improved
Stack trace preservation full stack trace printed silently lost (varargs overload) regression*
Message format raw Java stack dump SLF4J-style formatted message improved

*Pre-existing limitation in BaseJulLogger — affects all RecordLog.warn(format, param, e) calls in the codebase.

Follow-Ups

Type Item Blocking
Non-blocking Use the 2-arg RecordLog.warn("message", e) overload to preserve stack traces in the 3 changed catch blocks No
Non-blocking Add failure-injection tests for ConfigUtil catch blocks (currently zero coverage of error paths) No
Non-blocking Sign the Sentinel CLA — GitHub reports merge is blocked (process issue, not code quality) No

Recommendation

Approve with notes. This is a safe, low-risk logging-only change. Two independent runtime trace pairs prove zero contract divergence and confirm the head JVM starts without circular dependency issues. The main note for the PR author: due to a pre-existing BaseJulLogger limitation, the varargs RecordLog.warn(format, param, exception) overload silently discards the exception stack trace — using the 2-arg overload would better achieve the PR's goal of improved error diagnostics.

@lxcxjxhx lxcxjxhx closed this by deleting the head repository Jul 31, 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.

fix: replace printStackTrace with RecordLog in ConfigUtil

3 participants