Фильтрация мусорных логов #1420
Conversation
tests/dummy/app/templates/log-service-examples/settings-example.hbs
Outdated
Show resolved
Hide resolved
|
SonarCloud Quality Gate failed.
|
TeterinaSvetlana
changed the base branch from
feature-ember-update
to
develop
|
актуально? @pepelyaeva |
WalkthroughThe changes in the pull request focus on the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
addon/services/log.js (1)
Line range hint
414-521: Consider refactoring logging methods to reduce code duplicationThe
_checkMessageOnSkippedchecks and the subsequent logging logic are repeated across multipleEmber.Loggermethods (error,warn,log,info,debug). To improve maintainability, consider abstracting the common logic into a separate function or helper. This will reduce code duplication and make future updates easier.For example, you could create a new method that handles the message processing:
_processLogMessage(originalMethod, category, args) { originalMethod(...args); const message = joinArguments(...args); if (this._checkMessageOnSkipped(category, message)) return; return this._queue.attach((resolve, reject) => { return this._storeToApplicationLog(category, message, '').then((result) => { resolve(result); }).catch((reason) => { reject(reason); }); }); }Then update each
Ember.Loggermethod to use this helper:Ember.Logger.error = function() { - originalEmberLoggerError(...arguments); - const message = joinArguments(...arguments); - if (_this._checkMessageOnSkipped(messageCategory.error, message)) return; - return _this._queue.attach((resolve, reject) => { - return _this._storeToApplicationLog(messageCategory.error, message, '').then((result) => { - resolve(result); - }).catch((reason) => { - reject(reason); - }); - }); + _this._processLogMessage(originalEmberLoggerError, messageCategory.error, arguments); };Repeat similar changes for the other logging methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- addon/services/log.js (7 hunks)
🧰 Additional context used
🔇 Additional comments (7)
addon/services/log.js (7)
363-368: LGTMThe additions to the
errorMessageFiltersarray appropriately include additional messages to be filtered, enhancing the log filtering mechanism.
414-416: Adding message skip check toEmber.Logger.errorThe inclusion of
_this._checkMessageOnSkippedin theEmber.Logger.errormethod ensures that error messages are filtered according to the configured criteria before being logged.
437-442: Properly filtering deprecation and warning messages inEmber.Logger.warnThe addition of
_this._checkMessageOnSkippedchecks for both deprecation and warning messages in theEmber.Logger.warnmethod enhances the consistency of message filtering.
449-450: Ensuring warning messages are conditionally loggedThe check for
_this._checkMessageOnSkippedfor warning messages within the else block ofEmber.Logger.warnaligns with the intended selective logging behavior.
471-473: Incorporating skip check inEmber.Logger.logIncluding
_this._checkMessageOnSkippedin theEmber.Logger.logmethod allows for proper filtering of log messages based on the configuration.
494-496: Implementing message filtering inEmber.Logger.infoThe addition of the
_this._checkMessageOnSkippedcheck in theEmber.Logger.infomethod ensures informational messages are appropriately filtered.
517-519: Filtering debug messages inEmber.Logger.debugAdding
_this._checkMessageOnSkippedin theEmber.Logger.debugmethod correctly applies the filtering criteria to debug messages.
| if (errorMessageFilterActive) { | ||
| errorMessageFilters.forEach(errorMessageFilter => { | ||
| if (category.name === errorMessageFilter.category && message.indexOf(errorMessageFilter.message) !== -1) { | ||
| isSkippedMessage = true; | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize error message filtering with some method
In _checkMessageOnSkipped (lines 689-695), to improve performance and readability, consider replacing the forEach loop with the some method. This change allows the iteration to exit early when a matching filter is found.
Apply this diff to implement the change:
if (errorMessageFilterActive) {
- errorMessageFilters.forEach(errorMessageFilter => {
- if (category.name === errorMessageFilter.category && message.indexOf(errorMessageFilter.message) !== -1) {
- isSkippedMessage = true;
- }
- });
+ isSkippedMessage = errorMessageFilters.some(errorMessageFilter =>
+ category.name === errorMessageFilter.category &&
+ message.indexOf(errorMessageFilter.message) !== -1
+ );
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (errorMessageFilterActive) { | |
| errorMessageFilters.forEach(errorMessageFilter => { | |
| if (category.name === errorMessageFilter.category && message.indexOf(errorMessageFilter.message) !== -1) { | |
| isSkippedMessage = true; | |
| } | |
| }); | |
| } | |
| if (errorMessageFilterActive) { | |
| isSkippedMessage = errorMessageFilters.some(errorMessageFilter => | |
| category.name === errorMessageFilter.category && | |
| message.indexOf(errorMessageFilter.message) !== -1 | |
| ); | |
| } |
| _checkMessageOnSkipped(category, message) { | ||
| if (!this.get('enabled') || | ||
| category.name === messageCategory.error.name && !this.get('storeErrorMessages') || | ||
| category.name === messageCategory.warn.name && !this.get('storeWarnMessages') || | ||
| category.name === messageCategory.log.name && !this.get('storeLogMessages') || | ||
| category.name === messageCategory.info.name && !this.get('storeInfoMessages') || | ||
| category.name === messageCategory.debug.name && !this.get('storeDebugMessages') || | ||
| category.name === messageCategory.deprecate.name && !this.get('storeDeprecationMessages') || | ||
| category.name === messageCategory.promise.name && !this.get('storePromiseErrors')) { | ||
| new RSVP.Promise((resolve) => { | ||
| this._triggerEvent(category.name); | ||
| resolve(); | ||
| }); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Remove unnecessary Promise creation in _checkMessageOnSkipped
In the _checkMessageOnSkipped method (lines 669-683), the RSVP.Promise created at lines 678-681 is unnecessary as it is not used or returned. You can remove the Promise creation and directly call _triggerEvent.
Apply this diff to fix the issue:
if (!this.get('enabled') ||
category.name === messageCategory.error.name && !this.get('storeErrorMessages') ||
category.name === messageCategory.warn.name && !this.get('storeWarnMessages') ||
category.name === messageCategory.log.name && !this.get('storeLogMessages') ||
category.name === messageCategory.info.name && !this.get('storeInfoMessages') ||
category.name === messageCategory.debug.name && !this.get('storeDebugMessages') ||
category.name === messageCategory.deprecate.name && !this.get('storeDeprecationMessages') ||
category.name === messageCategory.promise.name && !this.get('storePromiseErrors')) {
- new RSVP.Promise((resolve) => {
- this._triggerEvent(category.name);
- resolve();
- });
+ this._triggerEvent(category.name);
return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _checkMessageOnSkipped(category, message) { | |
| if (!this.get('enabled') || | |
| category.name === messageCategory.error.name && !this.get('storeErrorMessages') || | |
| category.name === messageCategory.warn.name && !this.get('storeWarnMessages') || | |
| category.name === messageCategory.log.name && !this.get('storeLogMessages') || | |
| category.name === messageCategory.info.name && !this.get('storeInfoMessages') || | |
| category.name === messageCategory.debug.name && !this.get('storeDebugMessages') || | |
| category.name === messageCategory.deprecate.name && !this.get('storeDeprecationMessages') || | |
| category.name === messageCategory.promise.name && !this.get('storePromiseErrors')) { | |
| new RSVP.Promise((resolve) => { | |
| this._triggerEvent(category.name); | |
| resolve(); | |
| }); | |
| return true; | |
| } | |
| _checkMessageOnSkipped(category, message) { | |
| if (!this.get('enabled') || | |
| category.name === messageCategory.error.name && !this.get('storeErrorMessages') || | |
| category.name === messageCategory.warn.name && !this.get('storeWarnMessages') || | |
| category.name === messageCategory.log.name && !this.get('storeLogMessages') || | |
| category.name === messageCategory.info.name && !this.get('storeInfoMessages') || | |
| category.name === messageCategory.debug.name && !this.get('storeDebugMessages') || | |
| category.name === messageCategory.deprecate.name && !this.get('storeDeprecationMessages') || | |
| category.name === messageCategory.promise.name && !this.get('storePromiseErrors')) { | |
| this._triggerEvent(category.name); | |
| return true; | |
| } |
Summary by CodeRabbit
New Features
Improvements