ENG-10963 feat(log): add stdlib logging pipeline in reflex_base.utils.log (1/5) - #6863
ENG-10963 feat(log): add stdlib logging pipeline in reflex_base.utils.log (1/5)#6863FarhanAliRaza wants to merge 1 commit into
Conversation
Standard python logging with per-module loggers: rich-rendering console handler preserving the legacy colors, JSON-lines handler behind REFLEX_LOG_JSON, record dedupe, and file logging. LogLevel gains a correct total ordering (the str mixin compared alphabetically) and to_logging_level(). console.py delegates set_log_level into the pipeline and respects JSON mode in print/rule/status/progress; reflex bootstraps the reflex-owned loggers on import.
Greptile SummaryThis PR introduces a standard-library logging pipeline with Rich, JSON-lines, deduplication, and file sinks, then connects console helpers and top-level Reflex imports to it.
Confidence Score: 4/5The standalone reflex_base environment path should be fixed before merging because it can violate JSON logging and omit diagnostics from Reflex file capture. The logging pipeline works across its covered top-level Reflex paths, but the migrated dotenv error remains reachable before bootstrap and therefore escapes the configured sinks; print_table also lacks the JSON-mode behavior expected of neighboring console helpers. Files Needing Attention: packages/reflex-base/src/reflex_base/environment.py; packages/reflex-base/src/reflex_base/utils/console.py
|
| Filename | Overview |
|---|---|
| packages/reflex-base/src/reflex_base/utils/log.py | Adds the core logging pipeline, lazy bootstrap, handlers, deduplication, file logging, and deprecation utilities; standalone reflex_base initialization is not covered. |
| packages/reflex-base/src/reflex_base/environment.py | Adds REFLEX_LOG_JSON and migrates the dotenv dependency error to logging, which can bypass the pipeline on direct reflex_base imports. |
| packages/reflex-base/src/reflex_base/utils/console.py | Delegates level configuration and adapts interactive output for JSON mode, but the new print_table helper omits equivalent handling. |
| packages/reflex-base/src/reflex_base/constants/base.py | Implements rank-based LogLevel ordering and conversion to stdlib logging levels. |
| reflex/init.py | Bootstraps Reflex-owned loggers lazily during top-level reflex imports. |
| tests/units/reflex_base/utils/test_log.py | Provides broad coverage of formatting, levels, JSON output, deduplication, file logging, and bootstrap behavior, but not standalone reflex_base startup. |
Reviews (1): Last reviewed commit: "feat(log): add stdlib logging pipeline i..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| logger.error( | ||
| """The `python-dotenv` package is required to load environment variables from a file. Run `pip install "python-dotenv>=1.1.0"`.""" | ||
| ) |
There was a problem hiding this comment.
If REFLEX_ENV_FILE is set, python-dotenv is unavailable, and reflex_base.environment is imported without first importing reflex, this error uses an unclaimed stdlib logger. It emits plain stderr instead of a JSON record and bypasses Reflex file logging, breaking the configured logging contract.
Knowledge Base Used: Config and Environment System
| for row in tabular_data: | ||
| table.add_row(*row) | ||
|
|
||
| _console.print(table) |
There was a problem hiding this comment.
The new print_table helper writes Rich output unconditionally, unlike the neighboring console helpers that emit JSON or remain silent in JSON mode. Callers using it with REFLEX_LOG_JSON therefore add non-JSON text to the machine-readable stream and must implement their own special-case guard.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Merging this PR will not alter performance
Comparing 
Footnotes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c9743d5da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Returns: | ||
| A file handler writing every record with markup stripped. | ||
| """ | ||
| handler = logging.FileHandler(_log_file_path(), mode="w", encoding="utf-8") |
There was a problem hiding this comment.
When REFLEX_ENABLE_FULL_LOGGING=true, normal config initialization calls console.set_log_level(), which opens this new handler, while the still-active legacy helpers later open console.log_file_console() as a second writer. With REFLEX_LOG_FILE set, the legacy writer unlinks the already-open path (splitting subsequent records into an unreachable file on POSIX and potentially raising on Windows); without an override, the two writers usually create separate timestamped files. Reuse one handler/path so full logs remain complete.
Useful? React with 👍 / 👎.
| _write_json( | ||
| { | ||
| "timestamp": datetime.datetime.now(tz=datetime.timezone.utc).isoformat(), | ||
| "level": "info", |
There was a problem hiding this comment.
When REFLEX_LOG_JSON=true, the unmigrated console.warn(), console.error(), and console.success() helpers all eventually call emit_json_print(), so this hardcoded value serializes every legacy warning and error as "level": "info". Machine consumers filtering on severity will therefore miss failures; pass the originating level through the JSON emission path.
Useful? React with 👍 / 👎.
| if _log.is_json_mode(): | ||
| _log.emit_json_print(msg, dedupe=dedupe) | ||
| return |
There was a problem hiding this comment.
When REFLEX_LOG_JSON=true, this guard only covers console.print(): common flows such as reflex init call console.log(), which writes a timestamped Rich line directly through _console.log, and print_table() similarly writes rendered text directly. Those lines are not JSON objects, so a JSON-lines parser fails even though the mode promises machine-readable output; make all direct console writers JSON-aware or suppress them.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 13 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/reflex-base/src/reflex_base/utils/log.py">
<violation number="1" location="packages/reflex-base/src/reflex_base/utils/log.py:287">
P2: Full logging crashes when `REFLEX_LOG_FILE` targets a new directory because its parent is not created; create the configured path's parent before returning it.</violation>
<violation number="2" location="packages/reflex-base/src/reflex_base/utils/log.py:296">
P2: The new file handler here and the legacy `console.log_file_console()` writer both open their own log files when full logging is enabled, instead of sharing one handler/path. This can split log output across two files, or on POSIX cause the legacy writer to unlink the file this handler still has open, leaving subsequent records unrecoverable.</violation>
<violation number="3" location="packages/reflex-base/src/reflex_base/utils/log.py:445">
P2: The lazy `bootstrap()` design only stays cheap if no reflex-owned module logs during `import reflex`. `_BootstrapHandler.handle` calls `configure()` on the very first record, and `configure()` imports `reflex_base.environment` (dragging in the config/plugin stack plus pandas/plotly) — exactly what the `bootstrap()` docstring says it avoids. If any module in `ROOT_LOGGER_NAMES` emits a warning/debug record during its own import, the eager import happens anyway and `import reflex` is no longer lazy. This is a fragile invariant worth hardening (e.g. deferring the environment-sensitive `configure` or making the bootstrap handler itself gate on level so imports don't force the heavy path).</violation>
</file>
<file name="packages/reflex-base/src/reflex_base/utils/console.py">
<violation number="1" location="packages/reflex-base/src/reflex_base/utils/console.py:107">
P2: In JSON mode, `console.error(...)` (and `console.warn(...)`) now route through `emit_json_print(...)`, which hardcodes `"level": "info"` in the emitted record. So real errors and warnings — precisely the records a machine consumer of the JSON stream most needs to distinguish — are tagged as `info`. The `stderr=True` flag only selects the stream; the severity is lost. Consider passing an explicit level (e.g. `error`/`warning`) into `emit_json_print` based on the caller, so the JSON records preserve the legacy console severity.</violation>
<violation number="2" location="packages/reflex-base/src/reflex_base/utils/console.py:432">
P2: `print_table` corrupts JSON-mode stdout with Rich table text; suppress or serialize it when `REFLEX_LOG_JSON` is enabled, as `rule` already does.</violation>
</file>
<file name="reflex/__init__.py">
<violation number="1" location="reflex/__init__.py:101">
P2: On Python 3.10 this newly added deprecation warning is emitted during `import reflex`, and because `bootstrap()` has already attached `_BootstrapHandler` to the `reflex` root logger (with `propagate=False` and level DEBUG), the very first record routes through `_BootstrapHandler.handle` → `configure()` → `from reflex_base.environment import environment`. That eagerly imports the config/plugin stack (and pandas/plotly) at import time, which is exactly the lazy-loading regression the `bootstrap()` docstring in `reflex_base/utils/log.py` was written to avoid. The lazy attach only holds for Python 3.11+; 3.10 users take the heavy import path. Consider bypassing the pipeline here (e.g. `logging.getLogger(__name__)._log` via a plain root/Sys logging call that doesn't hit the bootstrap handler) or deferring the warning.</violation>
</file>
<file name="packages/reflex-base/src/reflex_base/environment.py">
<violation number="1" location="packages/reflex-base/src/reflex_base/environment.py:794">
P2: This error now goes through a plain `logging.getLogger(__name__)` call instead of `console.error()`. If `reflex_base.environment` is imported before `reflex` (so `log.bootstrap()` hasn't claimed this logger yet), the record falls back to default stdlib handling — plain stderr text instead of a JSON record, and it bypasses full-logging file capture — breaking the logging contract this PR establishes.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| from reflex_base.environment import environment | ||
|
|
||
| if env_log_file := environment.REFLEX_LOG_FILE.get(): | ||
| return env_log_file |
There was a problem hiding this comment.
P2: Full logging crashes when REFLEX_LOG_FILE targets a new directory because its parent is not created; create the configured path's parent before returning it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/reflex-base/src/reflex_base/utils/log.py, line 287:
<comment>Full logging crashes when `REFLEX_LOG_FILE` targets a new directory because its parent is not created; create the configured path's parent before returning it.</comment>
<file context>
@@ -0,0 +1,679 @@
+ from reflex_base.environment import environment
+
+ if env_log_file := environment.REFLEX_LOG_FILE.get():
+ return env_log_file
+ subseconds = int((time.time() % 1) * 1000)
+ timestamp = time.strftime("%Y-%m-%d_%H-%M-%S") + f"_{subseconds:03d}"
</file context>
| tabular_data: The data to print in tabular format. | ||
| headers: The headers for the table. | ||
| """ | ||
| table = Table() |
There was a problem hiding this comment.
P2: print_table corrupts JSON-mode stdout with Rich table text; suppress or serialize it when REFLEX_LOG_JSON is enabled, as rule already does.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/reflex-base/src/reflex_base/utils/console.py, line 432:
<comment>`print_table` corrupts JSON-mode stdout with Rich table text; suppress or serialize it when `REFLEX_LOG_JSON` is enabled, as `rule` already does.</comment>
<file context>
@@ -417,6 +419,27 @@ def ask(
+ tabular_data: The data to print in tabular format.
+ headers: The headers for the table.
+ """
+ table = Table()
+
+ for column in headers:
</file context>
| table = Table() | |
| if _log.is_json_mode(): | |
| return | |
| table = Table() |
| import logging | ||
|
|
||
| console.warn( | ||
| logging.getLogger(__name__).warning( |
There was a problem hiding this comment.
P2: On Python 3.10 this newly added deprecation warning is emitted during import reflex, and because bootstrap() has already attached _BootstrapHandler to the reflex root logger (with propagate=False and level DEBUG), the very first record routes through _BootstrapHandler.handle → configure() → from reflex_base.environment import environment. That eagerly imports the config/plugin stack (and pandas/plotly) at import time, which is exactly the lazy-loading regression the bootstrap() docstring in reflex_base/utils/log.py was written to avoid. The lazy attach only holds for Python 3.11+; 3.10 users take the heavy import path. Consider bypassing the pipeline here (e.g. logging.getLogger(__name__)._log via a plain root/Sys logging call that doesn't hit the bootstrap handler) or deferring the warning.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At reflex/__init__.py, line 101:
<comment>On Python 3.10 this newly added deprecation warning is emitted during `import reflex`, and because `bootstrap()` has already attached `_BootstrapHandler` to the `reflex` root logger (with `propagate=False` and level DEBUG), the very first record routes through `_BootstrapHandler.handle` → `configure()` → `from reflex_base.environment import environment`. That eagerly imports the config/plugin stack (and pandas/plotly) at import time, which is exactly the lazy-loading regression the `bootstrap()` docstring in `reflex_base/utils/log.py` was written to avoid. The lazy attach only holds for Python 3.11+; 3.10 users take the heavy import path. Consider bypassing the pipeline here (e.g. `logging.getLogger(__name__)._log` via a plain root/Sys logging call that doesn't hit the bootstrap handler) or deferring the warning.</comment>
<file context>
@@ -87,14 +87,21 @@
+ import logging
- console.warn(
+ logging.getLogger(__name__).warning(
"Reflex support for Python 3.10 is deprecated and will be removed in a future release. Please upgrade to Python 3.11 or higher for continued support."
)
</file context>
| kwargs: Keyword arguments to pass to the print function. | ||
| """ | ||
| if _log.is_json_mode(): | ||
| _log.emit_json_print(msg, dedupe=dedupe, stderr=True) |
There was a problem hiding this comment.
P2: In JSON mode, console.error(...) (and console.warn(...)) now route through emit_json_print(...), which hardcodes "level": "info" in the emitted record. So real errors and warnings — precisely the records a machine consumer of the JSON stream most needs to distinguish — are tagged as info. The stderr=True flag only selects the stream; the severity is lost. Consider passing an explicit level (e.g. error/warning) into emit_json_print based on the caller, so the JSON records preserve the legacy console severity.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/reflex-base/src/reflex_base/utils/console.py, line 107:
<comment>In JSON mode, `console.error(...)` (and `console.warn(...)`) now route through `emit_json_print(...)`, which hardcodes `"level": "info"` in the emitted record. So real errors and warnings — precisely the records a machine consumer of the JSON stream most needs to distinguish — are tagged as `info`. The `stderr=True` flag only selects the stream; the severity is lost. Consider passing an explicit level (e.g. `error`/`warning`) into `emit_json_print` based on the caller, so the JSON records preserve the legacy console severity.</comment>
<file context>
@@ -106,6 +103,9 @@ def _print_stderr(msg: str, *, dedupe: bool = False, **kwargs):
kwargs: Keyword arguments to pass to the print function.
"""
+ if _log.is_json_mode():
+ _log.emit_json_print(msg, dedupe=dedupe, stderr=True)
+ return
if dedupe:
</file context>
| @@ -0,0 +1,679 @@ | |||
| """Standard-library logging pipeline with rich rendering and JSON output. | |||
There was a problem hiding this comment.
P2: The lazy bootstrap() design only stays cheap if no reflex-owned module logs during import reflex. _BootstrapHandler.handle calls configure() on the very first record, and configure() imports reflex_base.environment (dragging in the config/plugin stack plus pandas/plotly) — exactly what the bootstrap() docstring says it avoids. If any module in ROOT_LOGGER_NAMES emits a warning/debug record during its own import, the eager import happens anyway and import reflex is no longer lazy. This is a fragile invariant worth hardening (e.g. deferring the environment-sensitive configure or making the bootstrap handler itself gate on level so imports don't force the heavy path).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/reflex-base/src/reflex_base/utils/log.py, line 445:
<comment>The lazy `bootstrap()` design only stays cheap if no reflex-owned module logs during `import reflex`. `_BootstrapHandler.handle` calls `configure()` on the very first record, and `configure()` imports `reflex_base.environment` (dragging in the config/plugin stack plus pandas/plotly) — exactly what the `bootstrap()` docstring says it avoids. If any module in `ROOT_LOGGER_NAMES` emits a warning/debug record during its own import, the eager import happens anyway and `import reflex` is no longer lazy. This is a fragile invariant worth hardening (e.g. deferring the environment-sensitive `configure` or making the bootstrap handler itself gate on level so imports don't force the heavy path).</comment>
<file context>
@@ -0,0 +1,679 @@
+ keeps records that a later ``configure`` may want; the sink's own level
+ does the real gating.
+ """
+ for name in ROOT_LOGGER_NAMES:
+ logger = logging.getLogger(name)
+ logger.propagate = False
</file context>
|
|
||
| if load_dotenv is None: | ||
| console.error( | ||
| logger.error( |
There was a problem hiding this comment.
P2: This error now goes through a plain logging.getLogger(__name__) call instead of console.error(). If reflex_base.environment is imported before reflex (so log.bootstrap() hasn't claimed this logger yet), the record falls back to default stdlib handling — plain stderr text instead of a JSON record, and it bypasses full-logging file capture — breaking the logging contract this PR establishes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/reflex-base/src/reflex_base/environment.py, line 794:
<comment>This error now goes through a plain `logging.getLogger(__name__)` call instead of `console.error()`. If `reflex_base.environment` is imported before `reflex` (so `log.bootstrap()` hasn't claimed this logger yet), the record falls back to default stdlib handling — plain stderr text instead of a JSON record, and it bypasses full-logging file capture — breaking the logging contract this PR establishes.</comment>
<file context>
@@ -781,13 +787,11 @@ def _load_dotenv_from_files(files: list[Path]):
if load_dotenv is None:
- console.error(
+ logger.error(
"""The `python-dotenv` package is required to load environment variables from a file. Run `pip install "python-dotenv>=1.1.0"`."""
)
</file context>
|
|
||
|
|
||
| @once | ||
| def _file_handler() -> logging.FileHandler: |
There was a problem hiding this comment.
P2: The new file handler here and the legacy console.log_file_console() writer both open their own log files when full logging is enabled, instead of sharing one handler/path. This can split log output across two files, or on POSIX cause the legacy writer to unlink the file this handler still has open, leaving subsequent records unrecoverable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/reflex-base/src/reflex_base/utils/log.py, line 296:
<comment>The new file handler here and the legacy `console.log_file_console()` writer both open their own log files when full logging is enabled, instead of sharing one handler/path. This can split log output across two files, or on POSIX cause the legacy writer to unlink the file this handler still has open, leaving subsequent records unrecoverable.</comment>
<file context>
@@ -0,0 +1,679 @@
+
+
+@once
+def _file_handler() -> logging.FileHandler:
+ """Create the full-logging file handler.
+
</file context>
masenf
left a comment
masenf
left a comment
There was a problem hiding this comment.
some legit reviewbot feedback here.
i'm going to skim the rest of the PRs to see if there's maybe some overlap i'm not seeing that will come together
| def __init__(self): | ||
| """Initialize the filter with an empty seen-set.""" | ||
| super().__init__() | ||
| self.seen: set = set() |
There was a problem hiding this comment.
probably just want to save the hash of the message, not the whole value. otherwise we leak all log messages that had dedupe set
| if progress is not None: | ||
| console = progress.console | ||
| end = getattr(record, "end", "\n") | ||
| console.print(f"{prefix}{record.getMessage()}", style=style, end=end) |
There was a problem hiding this comment.
does the rich markup in the message need to be escaped here to avoid getting formatting when we expected something with square brackets?
| if "[" not in msg: | ||
| return msg | ||
| try: | ||
| return Text.from_markup(msg).plain |
There was a problem hiding this comment.
not convinced that stripping markup is what we want for the JSON or file sinks:
>>> msg = "foo[bar]"
>>> Text.from_markup(msg).plain
'foo'
what we need to do is escape markdown in the log messages that we're printing via rich APIs and otherwise leave the log message alone for non-rich outputs
| handler.setFormatter( | ||
| _StripMarkupFormatter("[{asctime}] {levelname}: {message}", style="{") | ||
| ) |
There was a problem hiding this comment.
the file output should not be stripping out rich markup, because the framework shouldn't be putting rich markup directly into log records.
if log messages want to use colors, i think that should be a separate opt-in kwarg like rich=True and if you pass that, then you as the caller are responsible for calling rich.markup.escape on any non-markup data.
when we get a log record with rich=True (and only then), we should call strip_markup on it. otherwise everything else should come through unfiltered (with the whole message being passed through escape when writing to actual rich sink)
Then we can strip markup in a sane way while preserving already-escaped composed strings:
>>> msg = f"[b]Status: {rich.markup.escape('foo[bar]')}[/b]: Fizzed"
>>> Text.from_markup(msg).plain
'Status: foo[bar]: Fizzed'When we strip markup from a string that has already escaped rich markup, we get that original string back without the unescaped rich markup.
|
|
||
| def test_markup_rendered_in_rich_mode(capsys): | ||
| """Rich markup in messages is rendered, not shown literally.""" | ||
| logger.info("hello [bold]world[/bold]") |
There was a problem hiding this comment.
this really needs to be opt-in
| logger.info("hello [bold]world[/bold]") | |
| logger.info("hello [bold]world[/bold]", rich=True) |
| log.set_log_level(LogLevel.DEBUG) | ||
| import os | ||
|
|
||
| assert os.environ.get("REFLEX_LOGLEVEL") == "debug" |
There was a problem hiding this comment.
does the monkeypatch reset this? or does calling set_log_level leak an environment variable out of the test case?
|
|
||
| def test_set_log_level_none_is_noop(): | ||
| """Passing None keeps the current level.""" | ||
| log.set_log_level(None) |
There was a problem hiding this comment.
same story here. if the tests were invoked at debug level, it seems this test case would reset that back to default for the remaining cases
| def test_console_print_json_mode(monkeypatch, capsys): | ||
| """console.print stays machine-readable in JSON mode.""" | ||
| monkeypatch.setenv("REFLEX_LOG_JSON", "true") | ||
| console.print("plain [bold]message[/bold]") |
There was a problem hiding this comment.
| console.print("plain [bold]message[/bold]") | |
| console.print("AssertionErr: foo[bar] != 'baz'") |
an example of why rich needs to be opt in and stripping markup not the default for all log messages
| The logging pipeline deliberately disables propagation at package roots, | ||
| while pytest normally captures records from the process root logger. |
There was a problem hiding this comment.
to expand
i think we need a top level reflex logger that we do attach/propagate to root, and then attach the package level loggers to that such that we can keep a fairly standard setup, allow a single logger for programmatic tuning downstream, and when running through the reflex cli we should configure either the rich or json output; but if we're not in the cli, we should consider just NOT attaching a handler at all.
if we want --json mode to be useful, then we need to be careful about only emitting JSON; if we don't attach our handlers to the root logger when running in the cli, then other app code could break that contract by calling basicConfig or similar.
for apps that want to customize logging, they're probably wanting to attach systems to the root logger. i can see an advantage in having that intermediate logger below root, but having a changing list of potential non-propagating loggers that would need to be instrumented just seems annoying. this function is the first smell.
2 participants
Adds
reflex_base.utils.log: a standard pythonloggingpipeline with a rich-rendering console handler (legacy colors preserved), a JSON-lines handler behindREFLEX_LOG_JSON, record deduplication, and file logging.LogLevelgains a correct total ordering (thestrmixin compared alphabetically, so--loglevel criticalprinted the system-info banner) andto_logging_level().console.set_log_leveldelegates into the pipeline;print/rule/status/progressrespect JSON mode;print_tableadded.import reflexbootstraps the reflex-owned loggers (sinks attach lazily on the first record).Stack (ENG-10963)
this → #6864 → #6865 → #6866 → #6867.
Merge in order; each PR is based on the previous branch.