perf(RHINENG-24466): skip host_count query during MQ ingestion#3843
Open
rodrigonull wants to merge 1 commit intomasterfrom
Open
perf(RHINENG-24466): skip host_count query during MQ ingestion#3843rodrigonull wants to merge 1 commit intomasterfrom
rodrigonull wants to merge 1 commit intomasterfrom
Conversation
Contributor
SC Environment Impact AssessmentOverall Impact: ⚪ NONE No SC Environment-specific impacts detected in this PR. What was checkedThis PR was automatically scanned for:
|
3be1468 to
ec010c2
Compare
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/test_models.py" line_range="2843-2852" />
<code_context>
assert existing_host.display_name_reporter == "yupana"
+
+
+def test_serialize_group_with_host_count_false(flask_app, mocker): # noqa: ARG001
+ """serialize_group(with_host_count=False) should skip the host count query."""
+ from lib.group_repository import serialize_group
+
+ mock_group = mocker.Mock()
+ mock_group.id = "test-group-id"
+ mock_group.name = "Test Group"
+ mock_group.org_id = "test_org"
+
+ count_spy = mocker.patch("lib.group_repository.get_host_counts_batch")
+ mocker.patch(
+ "lib.group_repository.serialize_db_group_with_host_count",
+ return_value={"id": "test-group-id", "name": "Test Group", "host_count": 0},
+ )
+
+ result = serialize_group(mock_group, "test_org", with_host_count=False)
+
+ count_spy.assert_not_called()
+ assert result["host_count"] == 0
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for the RBAC workspace (`dict`) branch of `serialize_group`.
These tests only cover the DB/ORM `Group` path of `serialize_group`. Please also add tests for the RBAC v2 workspace (`dict`) path that:
1. Call `serialize_group` with a `dict` and `with_host_count=False`, asserting `get_host_counts_batch` is not called and `serialize_rbac_workspace_with_host_count` receives `host_count=0`.
2. Call `serialize_group` with a `dict` and `with_host_count=True` (or default), asserting `get_host_counts_batch` is called once with the expected arguments and that the returned `host_count` is included in the serialized result.
This will ensure both code paths behave consistently and the host-count optimization applies to RBAC workspaces as well.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+2843
to
+2852
| def test_serialize_group_with_host_count_false(flask_app, mocker): # noqa: ARG001 | ||
| """serialize_group(with_host_count=False) should skip the host count query.""" | ||
| from lib.group_repository import serialize_group | ||
|
|
||
| mock_group = mocker.Mock() | ||
| mock_group.id = "test-group-id" | ||
| mock_group.name = "Test Group" | ||
| mock_group.org_id = "test_org" | ||
|
|
||
| count_spy = mocker.patch("lib.group_repository.get_host_counts_batch") |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add coverage for the RBAC workspace (dict) branch of serialize_group.
These tests only cover the DB/ORM Group path of serialize_group. Please also add tests for the RBAC v2 workspace (dict) path that:
- Call
serialize_groupwith adictandwith_host_count=False, assertingget_host_counts_batchis not called andserialize_rbac_workspace_with_host_countreceiveshost_count=0. - Call
serialize_groupwith adictandwith_host_count=True(or default), assertingget_host_counts_batchis called once with the expected arguments and that the returnedhost_countis included in the serialized result.
This will ensure both code paths behave consistently and the host-count optimization applies to RBAC workspaces as well.
kruai
approved these changes
Mar 31, 2026
Collaborator
kruai
left a comment
There was a problem hiding this comment.
Looks good - host.groups doesn't have the group's host count, so we don't need to calculate it. Nice thinking!
Add with_host_count parameter to serialize_group() and pass False from add_host() during MQ ingestion. The ingress path only needs group metadata for the JSONB groups field, not an accurate host count. This eliminates 1 COUNT/JOIN query per message.
ec010c2 to
2dcb464
Compare
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
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.
Overview
This PR is being created to address RHINENG-24466.
Add with_host_count parameter to serialize_group() and pass False from add_host() during MQ ingestion. The ingress path only needs group metadata for the JSONB groups field, not an accurate host count. This eliminates 1 COUNT/JOIN query per message.
PR Checklist
Secure Coding Practices Documentation Reference
You can find documentation on this checklist here.
Secure Coding Checklist
Summary by Sourcery
Make group serialization optionally skip host count calculation and use this faster path during MQ host ingestion.
Enhancements:
Tests: