feat: file logging and configurable minimum log level#5
Closed
jpnurmi wants to merge 12 commits intogetsentry:getsentryfrom
Closed
feat: file logging and configurable minimum log level#5jpnurmi wants to merge 12 commits intogetsentry:getsentryfrom
jpnurmi wants to merge 12 commits intogetsentry:getsentryfrom
Conversation
Add a simplified base::OpenFile() (after upstream Chromium's helper of the same name) and use it to implement the previously stubbed LOG_TO_FILE destination. Callers pass the path via LoggingSettings::log_file_path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Otherwise, if OpenFile fails, InitLogging returns false but g_logging_destination is already updated to include LOG_TO_FILE, so subsequent log messages silently skip writing because g_log_file is null. On Windows where LOG_DEFAULT is LOG_TO_FILE, this would drop all logs when the caller ignores the failure. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Previously InitLogging would silently return true when settings.logging_dest contained LOG_TO_FILE but log_file_path was empty, leaving g_log_file null. Subsequent log messages routed to LOG_TO_FILE would then be silently dropped. Drop the empty-path guard so we call OpenFile regardless — it returns null for an empty path, which the following check reports as failure. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Open the new file into a local ScopedFILE first, then move it into g_log_file on success. This closes any previously opened file, and on failure leaves the prior state intact. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace the hardcoded `GetMinLogLevel()` with a runtime-configurable value. Add `SetMinLogLevel()` and a `min_log_level` field to `LoggingSettings` (applied by `InitLogging`). Default remains `LOG_INFO` so existing callers are unaffected. Reorder the `LogSeverity` constants above `LoggingSettings` so the struct's default initializer can reference `LOG_INFO`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jpnurmi
changed the title
Matches upstream Chromium. Truncation is unnecessary when callers already place the log file somewhere with a fresh lifetime, and append mode lets multiple processes share the same log file without a truncation race. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
InitLogging now only stores the path; the fopen is deferred until the first LOG call reaches the LOG_TO_FILE branch in Flush. Sessions that never log anything leave no file behind. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Wrap the lazy open and writes in Flush under a base::Lock so concurrent LOG calls can't race the file descriptor, and also lock the close + path update in InitLogging for symmetry. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Replace the INT_MAX stub in GetVlogLevel() with -GetMinLogLevel() so VLOG(N) fires iff the resulting severity (-N) passes the same bar as LOG(severity). Previously every VLOG call was unconditionally on. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Same-TU statics are destroyed in reverse declaration order, so with the previous order the lock would be destroyed before the ScopedFILE. If fclose() in ScopedFILECloser fails during teardown, the resulting PLOG(ERROR) re-enters Flush() and tries to acquire the already-destroyed lock. Swap the order so the lock outlives the file. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ScopedFILECloser emits PLOG(ERROR) if fclose fails, which re-enters Flush() and tries to acquire g_log_file_lock -- deadlock on a non-reentrant lock. Move the previous ScopedFILE out of the critical section so its destructor runs after the lock is released. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 942a0a7. Configure here.
This reverts commit e5efd70. Changing GetVlogLevel from INT_MAX to -GetMinLogLevel() silently disabled VLOG(N) for N >= 1 at the default LOG_INFO level, regressing every downstream VLOG caller. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@JoshuaMoelans I can't invite reviewers, but this would be ready for review whenever you've got a moment... 🙏 |
Collaborator
Author
|
Superseded by #6 |
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
1 participant
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.
Required for:
Two additions to mini_chromium's logging:
LOG_TO_FILEdestination. Implements the previously stubbedLOG_TO_FILEdestination and adds a simplifiedbase::OpenFile()helper (after upstream Chromium's helper of the same name). Callers pass the path viaLoggingSettings::log_file_path; the file is held open for the program's lifetime and closed by the destructor of a file-scopebase::ScopedFILEat exit.Configurable minimum log level. Replaces the hardcoded
GetMinLogLevel()with a runtime-configurable value. AddsSetMinLogLevel()and amin_log_levelfield onLoggingSettings(applied byInitLogging). Default remainsLOG_INFO, so existing callers are unaffected.See also: