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.

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

Fix/15634 inconsistent logging levels #15653

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

laky241
Copy link

@laky241 laky241 commented Feb 21, 2025

Fixes #15634

fixed inconsistent logging levels in two critical areas of the codebase, service worker error handling and screen-sharing failures

Previously, these error cases used console.log, making it difficult to identify failures during debugging.

@laky241 laky241 marked this pull request as draft February 21, 2025 17:50
@jitsi-jenkins
Copy link

Hi, thanks for your contribution!
If you haven't already done so, could you please make sure you sign our CLA (https://jitsi.org/icla for individuals and https://jitsi.org/ccla for corporations)? We would unfortunately be unable to merge your patch unless we have that piece :(.

@laky241
Copy link
Author

laky241 commented Feb 21, 2025

This PR only fixes some cases of inconsistent logging. There are likely more files where similar improvements can be made, I can expand the PR further based on the feedback.

@@ -1,3 +1,5 @@
import logger from './react/features/base/logging/logger';
Copy link
Member

Choose a reason for hiding this comment

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

Is there such a thing?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure this is correct at all ... have you tested this at all?
Thie is a service worker and is run outside of the app and is deliberately using console logging. Logger depends on configuration which is not enabled and available here.

Copy link
Author

Choose a reason for hiding this comment

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

thanks for the clarification! You’re absolutely right
I’ll revert the changes in pwa-worker.js

could you guide me on which files should be updated?
Would really appreciate your input before making further changes.

Copy link
Member

Choose a reason for hiding this comment

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

What problem are you trying to solve?

Copy link
Author

Choose a reason for hiding this comment

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

the main issue iam addressing here is inconsistent logging levels across the codebase. Right now, some errors are logged using console.log instead of console.error, which makes debugging harder. Additionally, some parts of the code use direct console statements instead of the logger utility,

Copy link
Member

Choose a reason for hiding this comment

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

Well what I mean, is to fix the issues you have experienced and find problematic.

Copy link
Member

Choose a reason for hiding this comment

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

The PWA worker is outside of the main jitsi meet app, so it doesn't follow the same rules. Please leave it out.

@@ -63,6 +63,7 @@ async function _startScreenSharing(dispatch: IStore['dispatch'], state: IReduxSt
}, NOTIFICATION_TIMEOUT_TYPE.LONG));
}
} catch (error: any) {
console.log('ERROR creating screen-sharing stream ', error);
console.error('ERROR creating screen-sharing stream ', error);
Copy link
Member

Choose a reason for hiding this comment

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

This should be using the logger indeed.

Also the ERROR is redundant since it's already implied by the log level.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent Logging Levels
4 participants