-
Notifications
You must be signed in to change notification settings - Fork 2
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
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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@v4
andsetup-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
filter
andfilter_ogs
options 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
filter
andfilter_ogs
options verified.The
filter
andfilter_ogs
configuration 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-ignore
improves 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/checkout
andactions/setup-python
have 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 offilter
property.The
filter
property correctly retrieves the WebSocket filtering configuration setting.
42-44
: Correct implementation offilter_ogs
property.The
filter_ogs
property 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
abf0f34
to
23c4905
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_ignored
list..github/workflows/install_tests.yml (2)
14-14
: LGTM!The changes to the
python-version
matrix 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
checkout
andsetup-python
actions 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-ignore
sections, 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/checkout
action to v4 is a good change that may bring improvements or new features.
42-42
: LGTM!Upgrading the
actions/setup-python
action 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
filter
andfilter_ogs
provide 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_message
method 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