[Proposal] set LogFormat to full on RxPlayer's debug mode
#1625
Conversation
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
Florent-Bouisset
approved these changes
Jan 9, 2025
peaBerberian
added a commit
to canalplus/RxPaired
that referenced
this pull request
Jan 9, 2025
This is an alternative attempt (after #22) at fixing the "double formatting" situation brought by canalplus/rx-player#1469 that may be made much more frequent by canalplus/rx-player#1625. This solution fixes it client-side instead, which could be seen as arguably less ugly. The idea is to rely on the fact that the RxPlayer does full formatting by calling log functions with at least three arguments: 1. The timestamp in a string format with always numbers after a comma 2. A "namespace" (e.g. "[warn]") 3-n. The log message's arguments By considering this, we can very easily detect client-side a probable case of full formatting.
peaBerberian
added a commit
to canalplus/RxPaired
that referenced
this pull request
Jan 9, 2025
This is an alternative attempt (after #22) at fixing the "double formatting" situation brought by canalplus/rx-player#1469 that may be made much more frequent by canalplus/rx-player#1625. This solution fixes it client-side instead, which could be seen as arguably less ugly. The idea is to rely on the fact that the RxPlayer does full formatting by calling log functions with at least three arguments: 1. The timestamp in a string format with always numbers after a comma 2. A "namespace" (e.g. "[warn]") 3-n. The log message's arguments By considering this, we can very easily detect client-side a probable case of full formatting.
The `__RX_PLAYER_DEBUG_MODE__` boolean -------------------------------------- The RxPlayer has a hidden feature where if a global `__RX_PLAYER_DEBUG_MODE__` boolean is declared globally and set to `true` before its code its imported, it will output debug logs. This was initially added as a mean to facilitate debugging, especially while relying on our [RxPaired](https://github.com/canalplus/RxPaired) inspector. Though we now end up communicating that trick to more people to facilitate quick checks: - applications relying on the RxPlayer which seem to have a fairly simple issue can set this to very quickly communicate us logs without having to understand our debug tools nor perform re-builds on their side. - in some scenarios, we even had users (though still in a professional context) wanting us to check why the player behaved the way it did on their devices. As those were technical people, we had no issue just telling them to set a boolean in the console and communicate to us the outputted logs. Problems with it ---------------- Logs are then generally just copy-pasted from e.g. Chrome's inspector, and as such have a lot of noise we don't need (call stack info, object structure from their own code, CORS from their applications etc.), no timestamp, and without a clear boundary between log lines vs line break in a single log. Thus, it's less convenient to us than relying on `RxPaired` or than re-building with both `LogLevel = "DEBUG"` **AND** `LogFormat = "full"`. Proposal -------- So I here propose that `RX_PLAYER_DEBUG_MODE__` also set `LogFormat` to `"full"`. This will add timestamps + a namespace to all new logs produced by the RxPlayer, then facilitating our exploitation and opening the way for easier import into RxPaired's post-debugger mode. Though to keep in mind that this will then always lead to `RxPaired`'s "double formatting" problem as exposed in canalplus/RxPaired#22. As such canalplus/RxPaired#22 or a similar work-around is a requirement before merging this. _Note that there are other solutions to fix this, like creating two global booleans, e.g. one used by RxPaired and one for users, but here I ended up preferring the ugly double formatting work-around as a lesser evil._
peaBerberian
force-pushed
the
misc/logformat-full-on-debug-mode
branch
from
b3f6e16 to
8c720a0
Compare
|
Automated performance checks have been performed on commit Tests results✅ Tests have passed. Performance tests 1st run outputNo significative change in performance for tests:
If you want to skip performance checks for latter commits, add the |
peaBerberian
added a commit
to canalplus/RxPaired
that referenced
this pull request
Mar 17, 2025
This is an alternative attempt (after #22) at fixing the "double formatting" situation brought by canalplus/rx-player#1469 that may be made much more frequent by canalplus/rx-player#1625. This solution fixes it client-side instead, which could be seen as arguably less ugly. The idea is to rely on the fact that the RxPlayer does full formatting by calling log functions with at least three arguments: 1. The timestamp in a string format with always numbers after a comma 2. A "namespace" (e.g. "[warn]") 3-n. The log message's arguments By considering this, we can very easily detect client-side a probable case of full formatting.
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The
__RX_PLAYER_DEBUG_MODE__booleanThe RxPlayer has a hidden feature where if an
__RX_PLAYER_DEBUG_MODE__boolean is declared globally and set totruebefore the RxPlayer's code is imported, it will output debug logs.This was initially added as a mean to facilitate debugging, especially while relying on our RxPaired inspector.
Though we now end up communicating that trick to more people to facilitate quick checks:
applications relying on the RxPlayer which seem to have a fairly simple issue can set this to very quickly communicate us logs without having to understand our debug tools nor perform re-builds on their side.
in some scenarios, we even had users (though still in a professional context) wanting us to check why the player behaved the way it did on their devices.
As those were technical people, we had no issue just telling them to set a boolean in the console and communicate to us the outputted logs.
Problems with it
Logs are then generally just copy-pasted from e.g. Chrome's inspector, and as such have a lot of noise we don't need (call stack info, object structure from their own code, CORS from their applications etc.), no timestamp, and no clear boundary between logs vs line breaks inside a single log.
Thus, it's less convenient to us than relying on
RxPairedor than re-building with bothLogLevel = "DEBUG"ANDLogFormat = "full".Proposal
So I here propose that
RX_PLAYER_DEBUG_MODE__also setLogFormatto"full".This will add timestamps + a namespace to all new logs produced by the RxPlayer, then facilitating our exploitation and opening the way for easier import into RxPaired's post-debugger mode.
Though to keep in mind that this will then always lead to
RxPaired's "double formatting" problem as exposed in canalplus/RxPaired#22.As such canalplus/RxPaired#22 or a similar work-around is a requirement before merging this.
Note that there are other solutions to fix this, like creating two global booleans, e.g. one used by RxPaired and one for users, but here I ended up preferring the ugly double formatting work-around as a lesser evil.