browser-client: Downgrade browser console log levels

tt2468 commented

2024-04-12 01:09:19 -05:00

(Migrated from github.com)

Description

Downgrades browser console log messages:

Error: LOG_WARNING -> LOG_DEBUG
Fatal: LOG_ERROR -> LOG_INFO

Motivation and Context

Throughout time, OBS has logged browser console.log errors as warnings in the OBS log as an easy way for browser source developers to find and fix issues with their pages. Unfortunately, this functionality seems to be almost completely ignored by many browser source providers. This logging does, however, clog up many OBS logs and result in readability issues when troubleshooting unrelated problems.

How Has This Been Tested?

No more log spam in my OBS log from random browser pages

Types of changes

Tweak (non-breaking change to improve existing functionality)

Checklist:

My code has been run through clang-format.
I have read the contributing document.
My code is not on the master branch.
The code has been tested.
All commit messages are properly formatted and commits squashed where appropriate.
I have included updates to all appropriate documentation.

### Description Downgrades browser console log messages: - Error: LOG_WARNING -> LOG_DEBUG - Fatal: LOG_ERROR -> LOG_INFO ### Motivation and Context Throughout time, OBS has logged browser console.log errors as warnings in the OBS log as an easy way for browser source developers to find and fix issues with their pages. Unfortunately, this functionality seems to be almost completely ignored by many browser source providers. This logging does, however, clog up many OBS logs and result in readability issues when troubleshooting unrelated problems. ### How Has This Been Tested? No more log spam in my OBS log from random browser pages ### Types of changes - Tweak (non-breaking change to improve existing functionality) ### Checklist: - [x] My code has been run through [clang-format](https://github.com/obsproject/obs-studio/blob/master/.clang-format). - [x] I have read the [**contributing** document](https://github.com/obsproject/obs-studio/blob/master/CONTRIBUTING.rst). - [x] My code is not on the master branch. - [x] The code has been tested. - [x] All commit messages are properly formatted and commits squashed where appropriate. - [x] I have included updates to all appropriate documentation.

👍 1

Warchamp7 commented

2024-04-12 09:00:17 -05:00

(Migrated from github.com)

Generally fine with this, but I think there would be value in another PR that added a summarized report to the log when a browser source is reloaded or destroyed, just so that this information isn't totally shoved under the rug.

Ex.

09:57:22.112: [obs-browser: 'Example Page'] Page reported 11 warnings and 24 errors. Enable verbose logging for detailed reporting.

Generally fine with this, but I think there would be value in another PR that added a summarized report to the log when a browser source is reloaded or destroyed, just so that this information isn't totally shoved under the rug. Ex. ``` 09:57:22.112: [obs-browser: 'Example Page'] Page reported 11 warnings and 24 errors. Enable verbose logging for detailed reporting. ```

alinsavix commented

2024-04-12 11:07:03 -05:00

(Migrated from github.com)

Is it worth considering having an option on the browser source to enable/disable this feature? I support a few streamers where sometimes I have to come in after-the-fact and figure out from the logs why something was misbehaving, and there have definitely been a few times where the logging from the browser sources let me do that -- so for those streamers, having the existing behavior is really valuable, and I could just have those streamers leave that value set appropriately, and I personally am willing to wade through the normal browser source spam to be able to have it there when I actually need it.

Fenrirthviti commented

2024-04-12 12:00:35 -05:00

(Migrated from github.com)

From off-thread discussions, I'd be far more in favor of just moving browser messages to a separate file and leaving everything else as-is.

Understandably, this comes with a different set of challenges for things like handling log uploads and such, but I personally feel that this has been such a pain to try and shove in the main OBS log for a long time now, and it's time to just separate it.

From off-thread discussions, I'd be far more in favor of just moving browser messages to a separate file and leaving everything else as-is. Understandably, this comes with a different set of challenges for things like handling log uploads and such, but I personally feel that this has been such a pain to try and shove in the main OBS log for a long time now, and it's time to just separate it.

WizardCM commented

2024-04-12 19:31:52 -05:00

(Migrated from github.com)

@Fenrirthviti This is possible here:

github.com/obsproject/obs-browser@b4f724ae6a/obs-browser-plugin.cpp (L312-L313)

LOGSEVERITY_DISABLE would need to be changed to LOGSEVERITY_WARNING, and we'd probably want to give it a nicer filename than debug.log (stored in the same location as the cookie directories).

@Fenrirthviti This is possible here: https://github.com/obsproject/obs-browser/blob/b4f724ae6abd371f8f0378f29c908f51065190f3/obs-browser-plugin.cpp#L312-L313 `LOGSEVERITY_DISABLE` would need to be changed to `LOGSEVERITY_WARNING`, and we'd probably want to give it a nicer filename than `debug.log` (stored in the same location as the cookie directories).

Fenrirthviti commented

2024-04-13 12:50:46 -05:00

(Migrated from github.com)

Sure, that part is easy, the concern would be adding a new option to upload the browser log file for support purposes. We do far less sanitization on browser logging than we do our own logging, so uploading that to us would make me slightly nervous from a privacy standpoint, but it's just shunting what we're already logging/uploading as part of the main log anyway, so it should be fine. We should have a warning on upload or something, but that can be discussed off-thread here.

As I'm against this specific PR as a solution, I'm voting in favor of closing this and moving the logging/uploading to a separate file.

Sure, that part is easy, the concern would be adding a new option to upload the browser log file for support purposes. We do far less sanitization on browser logging than we do our own logging, so uploading that to us would make me slightly nervous from a privacy standpoint, but it's just shunting what we're already logging/uploading as part of the main log anyway, so it should be fine. We should have a warning on upload or something, but that can be discussed off-thread here. As I'm against this specific PR as a solution, I'm voting in favor of closing this and moving the logging/uploading to a separate file.

This pull request is broken due to missing fork information.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin 'tt2468/fix/browser-logging:tt2468/fix/browser-logging'

git switch 'tt2468/fix/browser-logging'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'master'

git merge --no-ff 'tt2468/fix/browser-logging'

git switch 'tt2468/fix/browser-logging'

git rebase 'master'

git switch 'master'

git merge --ff-only 'tt2468/fix/browser-logging'

git switch 'tt2468/fix/browser-logging'

git rebase 'master'

git switch 'master'

git merge --no-ff 'tt2468/fix/browser-logging'

git switch 'master'

git merge --squash 'tt2468/fix/browser-logging'

git switch 'master'

git merge --ff-only 'tt2468/fix/browser-logging'

git switch 'master'

git merge 'tt2468/fix/browser-logging'

git push origin 'master'

Rows
Columns

browser-client: Downgrade browser console log levels #440

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Checkout

Merge