feat(performance): configurable filter #29
Conversation
|
Warning Rate limit exceeded@mikejgray has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 50 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe recent changes enhance the GitHub Actions workflows by updating Python version support, upgrading action versions for improved performance, and introducing new configuration options for message filtering in the Mycroft framework. These adjustments aim to improve security auditing during CI/CD processes and refine message handling, ensuring better communication between components. Changes
Assessment against linked issues
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: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- .github/workflows/build_tests.yml (1 hunks)
- .github/workflows/install_tests.yml (2 hunks)
- .github/workflows/notify_matrix.yml (1 hunks)
- .github/workflows/unit_tests.yml (2 hunks)
- README.md (1 hunks)
- ovos_messagebus/event_handler.py (1 hunks)
Files skipped from review due to trivial changes (1)
- .github/workflows/notify_matrix.yml
Additional comments not posted (9)
.github/workflows/build_tests.yml (1)
16-16: Verify the necessity of ignoring advisoryGHSA-9wx4-h78v-vm56.Ignoring security advisories can have implications on the security posture of the project. Ensure that the decision to ignore this advisory is well-justified and documented.
.github/workflows/install_tests.yml (2)
14-14: Approve Python version updates.Updating the Python version matrix to include newer versions ensures broader compatibility and access to new features.
17-19: Approve GitHub Actions updates.Upgrading to the latest versions of GitHub Actions (
checkout@v4andsetup-python@v5) can provide performance improvements and new features.Ensure that the updated actions are compatible with your workflows and do not introduce breaking changes.
README.md (1)
26-29: Approve documentation of new configuration options.The addition of
filterandfilter_ogsoptions provides enhanced control over message filtering. Ensure that the implementation aligns with the documented behavior.Verify that these options are correctly implemented and functional within the codebase.
Verification successful
Implementation of
filterandfilter_ogsoptions verified.The
filterandfilter_ogsconfiguration options are correctly implemented inovos_messagebus/event_handler.py, aligning with the documented behavior in the README. They are used to control message filtering effectively.
filter: Determines whether to emit messages.filter_ogs: Checks message types against a list to filter out specific types.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the new configuration options in the codebase. # Test: Search for the implementation of `filter` and `filter_ogs` options. # Expect: Code handling these configuration options as described in the documentation. ast-grep --lang python --pattern $'$_ = {\n "filter": $_,\n "filter_ogs": $_\n}'Length of output: 80
Script:
#!/bin/bash # Search for the usage of `filter` and `filter_ogs` in the codebase, # focusing on function calls or configuration handling logic. rg 'filter' -A 5 rg 'filter_ogs' -A 5Length of output: 3327
.github/workflows/unit_tests.yml (3)
7-15: Consistent use of double quotes for paths.The change to use double quotes for paths in
paths-ignoreimproves consistency across the file.Also applies to: 20-29
37-37: Updated Python version matrix.The Python version matrix now supports 3.8 to 3.12, which ensures compatibility with newer versions and removes deprecated 3.7.
However, verify that the codebase is compatible with these Python versions.
40-42: Upgraded GitHub Actions versions.The
actions/checkoutandactions/setup-pythonhave been upgraded to v4 and v5, respectively, which should improve performance and security.Verify that there are no breaking changes in the new action versions.
ovos_messagebus/event_handler.py (2)
38-40: Correct implementation offilterproperty.The
filterproperty correctly retrieves the WebSocket filtering configuration setting.
42-44: Correct implementation offilter_ogsproperty.The
filter_ogsproperty correctly retrieves the list of message types subject to filtering.
| @property | ||
| def max_message_size(self) -> int: | ||
| return Configuration().get("websocket", {}).get("max_msg_size", 10) * 1024 * 1024 | ||
|
|
||
| def on_message(self, message): | ||
| if not self.filter: |
There was a problem hiding this comment.
This could be simplified making if self.filter the conditional and leaving the common code here outside of the conditional
There was a problem hiding this comment.
Fair point. I put it on top because I wanted the default use case to be the first bit of code you read, rather than having to search down the method to find out what it does in a normal scenario.
|
rebase please :) |
The extra deserialization and serialization involved in filtering messages adds overhead to every message. By making it configurable, we could get some performance gains.
mikejgray
force-pushed
the
FEAT_ConfigurableFilter
branch
from
abf0f34
to
23c4905
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
.github/workflows/unit_tests.yml (1)
37-37: Use single quotes for the newer Python versions.To maintain consistency with the existing Python versions in the matrix, please enclose the newer versions (3.10, 3.11, and 3.12) in single quotes.
Apply this diff:
-python-version: [3.8, 3.9, "3.10", "3.11", "3.12"] +python-version: [3.8, 3.9, '3.10', '3.11', '3.12']
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- .github/workflows/build_tests.yml (1 hunks)
- .github/workflows/install_tests.yml (2 hunks)
- .github/workflows/unit_tests.yml (2 hunks)
- README.md (1 hunks)
- ovos_messagebus/event_handler.py (1 hunks)
Additional comments not posted (10)
.github/workflows/build_tests.yml (1)
16-16: Justify ignoring the security advisory "GHSA-9wx4-h78v-vm56".The change adds a new security advisory "GHSA-9wx4-h78v-vm56" to the list of ignored vulnerabilities during the audit process.
Ignoring security advisories should be done with caution and proper justification to avoid potential security risks.
Please provide the rationale for ignoring this specific advisory. If there is no valid reason, consider removing it from the
pipaudit_ignoredlist..github/workflows/install_tests.yml (2)
14-14: LGTM!The changes to the
python-versionmatrix are approved.Supporting newer Python versions (3.11 and 3.12) enhances compatibility and allows leveraging modern features. Removing the older version (3.7) helps focus testing efforts on more relevant versions.
17-17: LGTM!The upgrades to the
checkoutandsetup-pythonactions are approved.Upgrading actions to newer versions is generally beneficial for improvements, bug fixes, and new features.
Also applies to: 19-19
README.md (2)
27-29: LGTM!The new configuration options
"filter"and"filter_ogs"are approved.These options enhance the configurability of the WebSocket connections, allowing for more granular control over the types of messages that are processed by the skills. The default values provide a reasonable starting point for the filtering behavior.
29-29: There is an existing comment thread discussing the naming of"filter_ogs", suggesting that it might be a typo for"filter_logs". Generating a similar comment would be a duplicate of the ongoing discussion..github/workflows/unit_tests.yml (3)
7-15: LGTM!The changes to the
paths-ignoresections, replacing single quotes with double quotes for paths, are consistent and do not introduce any issues.Also applies to: 20-29
40-40: LGTM!Upgrading the
actions/checkoutaction to v4 is a good change that may bring improvements or new features.
42-42: LGTM!Upgrading the
actions/setup-pythonaction to v5 is a good change that may bring improvements or new features.ovos_messagebus/event_handler.py (2)
38-44: LGTM!The new properties
filterandfilter_ogsprovide a clean way to control message filtering based on configuration settings. Good addition!
51-76: Simplify the conditional logic inon_message.The changes in the
on_messagemethod enhance the control flow by introducing conditional logic based on configuration settings. However, as mentioned in the existing comments, the logic can be simplified to improve readability.Here's a proposed simplification:
def on_message(self, message): if not self.filter: try: self.emitter.emit(message) except Exception as e: LOG.exception(e) traceback.print_exc(file=sys.stdout) return try: deserialized_message = Message.deserialize(message) except Exception: return if deserialized_message.msg_type not in self.filter_ogs: LOG.debug(deserialized_message.msg_type + f' source: {deserialized_message.context.get("source", [])}' + f' destination: {deserialized_message.context.get("destination", [])}\n' f'SESSION: {SessionManager.get(deserialized_message).serialize()}') try: self.emitter.emit(deserialized_message.msg_type, deserialized_message) except Exception as e: LOG.exception(e) traceback.print_exc(file=sys.stdout) for client in client_connections: client.write_message(message)This simplification reduces nesting, eliminates redundant error handling, and improves readability while preserving the intended functionality.
|
fix the typo in "filter_ogs" too ? |
The extra deserialization and serialization involved in filtering messages adds overhead to every message. By making it configurable, we could get some performance gains.
Closes #23
Summary by CodeRabbit
Summary by CodeRabbit
New Features
"filter"(boolean) to determine filtering status."filter_logs"(array) to specify message types for filtering.Updates
Bug Fixes