Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: bring --enable-logging functionality in line with Chromium #25089

Merged
merged 62 commits into from Jun 17, 2021
Merged

feat: bring --enable-logging functionality in line with Chromium #25089

merged 62 commits into from Jun 17, 2021

Conversation

ckerr
Copy link
Member

@ckerr ckerr commented Aug 21, 2020 •
edited by nornagon

Description of Change

This adds support for several features of Chromium's logging system that were previously not exposed in Electron. In particular:

  • It's now possible to send Chromium logs to a file. This can be done either by passing --log-file=.../path/to/file.log along with --enable-logging. Passing --enable-logging=file without --log-file will send logs to a default path, electron_debug.log in the user-data directory.
    • This is particularly relevant on Windows, on which OS it is not possible to collect log messages from child processes when logging to stderr.
    • ELECTRON_ENABLE_LOGGING=[1|file] is equivalent to passing --enable-logging / --enable-logging=file
    • ELECTRON_LOG_FILE=.../path/to/file.log is equivalent to passing --log-file=.../path/to/file.log
  • We now support --log-level=n to set the minimum level of log messages printed.
  • It's now possible to enable logging from JavaScript, if done in the first tick. Logging switches added by app.commandLine.appendSwitch() will be read immediately after the first run of JS.

CC'ing @electron/wg-api

Checklist

  • PR description included and stakeholders cc'd
  • npm test passes
  • relevant documentation is changed or added
  • PR title follows semantic commit guidelines
  • PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.
  • This is NOT A BREAKING CHANGE. Breaking changes may not be merged to master until 11-x-y is branched.

Release Notes

Notes: Added support for directing Chromium logging to a file with --log-file=.../path/to/file.log. Also, it's now possible to enable logging from JavaScript by appending command-line switches during the first JS tick.

@ckerr
feat: support ELECTRON_LOG_FILE environment vars.
c6bce7d 
Adds support for two new environment variables, ELECTRON_LOG_FILE and
ELECTRON_LOG_LEVEL. If either of these is used, ELECTRON_ENABLE_LOGGING
is implied.
@ckerr
docs: document ELECTRON_LOG_FILE envvar
412168d
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Aug 21, 2020
@deepak1556
Copy link
Member

deepak1556 commented Aug 22, 2020 •
edited

Thanks for adding this, can we also implement kLogFile. It will match the following mapping in the end.

ELECTRON_ENABLE_LOGGING => --enable-logging
ELECTRON_LOG_FILE => --log-file

Personally I prefer to use command line flags, so it would help my case :)

@ckerr
chore: make lint happy
e7922b8
@ckerr
feat: add --log-file --log-level switches.
2716c2f 
These behave identically to the ELECTRON_LOG_FILE and ELECTRON_LOG_LEVEL
env variables
@ckerr
Copy link
Member Author

ckerr commented Aug 22, 2020

@deepak1556 I added kLogFile support.

For command-line verbosity I'd like your feedback on the other change here, a new --log-level switch, since -v and --vmodule are already baked into VlogInfo via logging::InitLogging() and aren't coupled to LogSeverity...

@ckerr
Copy link
Member Author

ckerr commented Aug 22, 2020 •
edited

While we're on the subject, do you know if that #if defined(OS_WIN) && defined(DEBUG) block still needed? It was introduced in this commit but it looks like we don't define DEBUG in any of our electron GN files -- so it's possible this hasn't been live since we moved from gyp to GN... shudder

@deepak1556
Copy link
Member

yup it can be removed, we don't define DEBUG and the code behind that block is now superseded with command line flags and env vars.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Aug 22, 2020
@jkleinsc
Copy link
Contributor

The @electron/wg-api approved this at the August 24, 2020 meeting

@ckerr ckerr changed the title feat: support ELECTRON_LOG_FILE environment vars feat: control log verbosity, logging destination via command line Aug 25, 2020
@ckerr
refactor: apply API WG's feedback to logging patch
4957164 
Basically the request was to stick closer to upstream's logging beahvior.

Headless' code was pretty flexible and also pretty close to what this
patch was trying to do, so I've updated this patch to adopt that code
wholesale.

Changes relative to previous version of this PR:
* env vars have been dropped
* --logging-enabled now takes an optional arg: filename or 'stderr'
* logfiles are saved in the same directory as electron exec
* upstream switch --logging-level replaces --log-level

Changes relative to headless:
* keep support for ELECTRON_ENABLE_LOGGING envvar;
  it is now equivalent to --logging-enabled=stderr
* remove the ifdef-NDEBUG-use-DIR_USER_DATA code because
  DIR_USER_DATA isn't initialized by the time we reach InitLogging
* omit code that uses the kProcessType switch because we don't use it?
@deepak1556 deepak1556 mentioned this pull request Aug 25, 2020
Closed
6 tasks
@ckerr ckerr changed the title feat: control log verbosity, logging destination via command line feat: copy headless' use of logging command-line switches Aug 26, 2020
@codebytere
Copy link
Member

@ckerr could you fix this up so we can merge?

@ckerr ckerr requested review from nornagon and a team as code owners October 1, 2020 19:36
@jkleinsc jkleinsc requested a review from deepak1556 June 15, 2021 20:03
deepak1556
deepak1556 approved these changes Jun 17, 2021
View reviewed changes
Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

LGTM

@nornagon nornagon merged commit 8ccab4c into main Jun 17, 2021
@nornagon nornagon deleted the refactor-logging branch June 17, 2021 21:17
@release-clerk
Copy link

release-clerk bot commented Jun 17, 2021

Release Notes Persisted

Added support for directing Chromium logging to a file with --log-file=.../path/to/file.log. Also, it's now possible to enable logging from JavaScript by appending command-line switches during the first JS tick.

@trop
Copy link
Contributor

trop bot commented Jun 17, 2021

I was unable to backport this PR to "14-x-y" cleanly;
you will need to perform this backport manually.

@nornagon nornagon mentioned this pull request Jun 30, 2021
Merged
@trop
Copy link
Contributor

trop bot commented Jun 30, 2021

@nornagon has manually backported this PR to "14-x-y", please check out #29963

nornagon added a commit that referenced this pull request Jun 30, 2021
@ckerr @nornagon
feat: bring --enable-logging functionality in line with Chromium (#25089
b905a88 
)

Co-authored-by: Jeremy Rose <jeremya@chromium.org>
zcbenz pushed a commit that referenced this pull request Jul 5, 2021
@nornagon @ckerr
feat: bring --enable-logging functionality in line with Chromium (#25089
aab2479 
) (#29963)

Co-authored-by: Jeremy Rose <jeremya@chromium.org>

Co-authored-by: Charles Kerr <charles@charleskerr.com>
@miniak miniak mentioned this pull request Jul 12, 2021
Merged
3 tasks
BlackHole1 pushed a commit to BlackHole1/electron that referenced this pull request Aug 30, 2021
@ckerr @nornagon @BlackHole1
feat: bring --enable-logging functionality in line with Chromium (ele…
ab15763 
…ctron#25089)

Co-authored-by: Jeremy Rose <jeremya@chromium.org>
@miniak miniak mentioned this pull request Sep 10, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarshallOfSound MarshallOfSound MarshallOfSound left review comments

@jkleinsc jkleinsc jkleinsc approved these changes

@deepak1556 deepak1556 deepak1556 approved these changes

@nornagon nornagon Awaiting requested review from nornagon

Assignees
No one assigned
Labels
api-review/approved ✅ api-review/requested 🗳 semver/minor backwards-compatible functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@ckerr @deepak1556 @jkleinsc @codebytere @nornagon @MarshallOfSound