PR Incidents Filtering for better CFR and MTTR Reporting #656
Conversation
WalkthroughThese changes introduce support for incident pull request (PR) filtering and configuration throughout the analytics backend. New models, settings, and adapters enable teams to include or exclude certain PRs as incidents, with customizable filters. The incident service and related APIs are updated to retrieve, process, and merge PR-based incidents alongside traditional incidents. New test cases validate these behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant IncidentService
participant CodeRepoService
participant SettingsService
Client->>API: Request incidents (with pr_filter)
API->>IncidentService: get_team_incidents(team_id, interval, pr_filter)
IncidentService->>SettingsService: Fetch PR incident settings
IncidentService->>CodeRepoService: Fetch PRs matching filters
IncidentService->>IncidentService: Adapt PRs to incidents
IncidentService->>IncidentService: Merge PR-based and traditional incidents
IncidentService-->>API: Return combined incidents
API-->>Client: Respond with filtered incidents
Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 10
🧹 Nitpick comments (1)
backend/analytics_server/mhq/service/incidents/incidents.py (1)
135-147: Avoid shadowing built-ins & improve loop efficiencyUsing
for filter in …shadows Python’s built-infilter, and scanning the entire filter list for every resolution PR is O(n²).- for pr in resolution_prs: - for filter in incident_prs_setting.filters: - pr_number = self._extract_pr_number_from_regex( - getattr(pr, filter["field"]), filter["value"] + for pr in resolution_prs: + for rule in incident_prs_setting.filters: + pr_number = self._extract_pr_number_from_regex( + getattr(pr, rule.field), rule.value # once adapters return objects )Consider pre-compiling the regex patterns once and/or indexing
repo_id_to_pr_number_to_pr_mapby repo & number to keep the lookup O(1) (you already do this).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (14)
backend/analytics_server/mhq/api/incidents.py(7 hunks)backend/analytics_server/mhq/api/resources/settings_resource.py(2 hunks)backend/analytics_server/mhq/service/code/pr_filter.py(3 hunks)backend/analytics_server/mhq/service/incidents/incident_filter.py(2 hunks)backend/analytics_server/mhq/service/incidents/incidents.py(4 hunks)backend/analytics_server/mhq/service/incidents/models/adapter.py(1 hunks)backend/analytics_server/mhq/service/settings/configuration_settings.py(7 hunks)backend/analytics_server/mhq/service/settings/default_settings_data.py(1 hunks)backend/analytics_server/mhq/service/settings/models.py(1 hunks)backend/analytics_server/mhq/service/settings/setting_type_validator.py(1 hunks)backend/analytics_server/mhq/store/models/code/filter.py(2 hunks)backend/analytics_server/mhq/store/models/settings/configuration_settings.py(1 hunks)backend/analytics_server/tests/service/Incidents/test_incident_types_setting.py(1 hunks)backend/analytics_server/tests/service/Incidents/test_mean_time_to_recovery.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (10)
backend/analytics_server/tests/service/Incidents/test_mean_time_to_recovery.py (2)
backend/analytics_server/mhq/service/incidents/incidents.py (1)
IncidentService(36-322)backend/analytics_server/tests/factories/models/incidents.py (1)
get_mean_time_to_recovery_metrics(101-104)
backend/analytics_server/mhq/service/settings/setting_type_validator.py (1)
backend/analytics_server/mhq/store/models/settings/configuration_settings.py (1)
SettingType(14-22)
backend/analytics_server/mhq/service/settings/default_settings_data.py (1)
backend/analytics_server/mhq/store/models/settings/configuration_settings.py (1)
SettingType(14-22)
backend/analytics_server/mhq/service/code/pr_filter.py (2)
backend/analytics_server/mhq/service/settings/models.py (2)
ExcludedPRsSetting(30-31)IncidentPRsSetting(56-58)backend/analytics_server/mhq/store/models/settings/configuration_settings.py (1)
SettingType(14-22)
backend/analytics_server/mhq/store/models/code/filter.py (1)
backend/analytics_server/mhq/store/models/code/pull_requests.py (1)
PullRequest(12-76)
backend/analytics_server/mhq/api/resources/settings_resource.py (1)
backend/analytics_server/mhq/service/settings/models.py (1)
IncidentPRsSetting(56-58)
backend/analytics_server/mhq/service/incidents/incident_filter.py (3)
backend/analytics_server/mhq/service/settings/models.py (1)
IncidentPRsSetting(56-58)backend/analytics_server/mhq/store/models/incidents/enums.py (1)
IncidentType(29-32)backend/analytics_server/mhq/store/models/settings/configuration_settings.py (1)
SettingType(14-22)
backend/analytics_server/tests/service/Incidents/test_incident_types_setting.py (4)
backend/analytics_server/mhq/service/incidents/incident_filter.py (3)
ConfigurationsIncidentFilterProcessor(65-126)apply(32-42)apply(80-89)backend/analytics_server/mhq/store/models/settings/configuration_settings.py (1)
SettingType(14-22)backend/analytics_server/mhq/service/settings/models.py (1)
IncidentTypesSetting(35-36)backend/analytics_server/mhq/store/models/incidents/enums.py (1)
IncidentType(29-32)
backend/analytics_server/mhq/service/settings/configuration_settings.py (2)
backend/analytics_server/mhq/service/settings/models.py (1)
IncidentPRsSetting(56-58)backend/analytics_server/mhq/store/models/settings/configuration_settings.py (1)
SettingType(14-22)
backend/analytics_server/mhq/service/incidents/incidents.py (11)
backend/analytics_server/mhq/service/settings/models.py (1)
IncidentPRsSetting(56-58)backend/analytics_server/mhq/store/models/code/filter.py (1)
PRFilter(10-108)backend/analytics_server/mhq/store/models/code/pull_requests.py (1)
PullRequest(12-76)backend/analytics_server/mhq/store/repos/code.py (2)
get_active_team_repos_by_team_id(263-268)get_prs_merged_in_interval(287-308)backend/analytics_server/mhq/utils/time.py (4)
time_now(10-11)Interval(14-96)from_time(26-27)to_time(30-31)backend/analytics_server/mhq/service/settings/configuration_settings.py (3)
SettingsService(22-490)get_settings_service(493-494)get_settings(119-131)backend/analytics_server/mhq/store/repos/incidents.py (1)
IncidentsRepoService(23-211)backend/analytics_server/mhq/service/incidents/models/adapter.py (2)
IncidentPRAdapter(6-24)adapt(8-24)backend/analytics_server/mhq/service/code/pr_filter.py (1)
apply_pr_filter(11-34)backend/analytics_server/mhq/service/incidents/incident_filter.py (1)
apply_incident_filter(45-62)backend/analytics_server/mhq/store/models/settings/configuration_settings.py (1)
SettingType(14-22)
🪛 GitHub Actions: Unit Tests
backend/analytics_server/tests/service/Incidents/test_incident_types_setting.py
[error] 4-4: ImportError: cannot import name 'IncidentPrsSetting' from 'mhq.service.settings.configuration_settings'. This caused test collection to fail.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: All file linting
🔇 Additional comments (27)
backend/analytics_server/tests/service/Incidents/test_mean_time_to_recovery.py (2)
19-19: Constructor updated correctly to match new signatureThe instantiation of
IncidentServicehas been properly updated to include the new requiredcode_repo_serviceparameter (set toNonefor testing purposes), matching the updated constructor signature.
27-27: Constructor updated correctly to match new signatureThe instantiation of
IncidentServicehas been properly updated to include the new requiredcode_repo_serviceparameter (set toNonefor testing purposes), matching the updated constructor signature.backend/analytics_server/mhq/store/models/settings/configuration_settings.py (1)
20-20: New setting type added correctlyThe addition of
INCIDENT_PRS_SETTINGto theSettingTypeenum follows the existing pattern and is placed in the appropriate location per the code comment guidance.backend/analytics_server/mhq/service/settings/setting_type_validator.py (1)
22-24: Validator properly extended for new setting typeThe conditional check for the new
INCIDENT_PRS_SETTINGvalue has been implemented correctly following the existing pattern in the validator function.backend/analytics_server/mhq/service/settings/default_settings_data.py (1)
32-37: Default configuration for incident PRs setting looks goodThe default settings provided for the new
INCIDENT_PRS_SETTINGtype are sensible and aligned with the PR objectives:
include_revert_prsdefaults toFalse, which matches the requirement to exclude REVERT_PR from incident typesfiltersis initialized as an empty array, ready to store regex filters for Incident PRsThe implementation follows the existing pattern in the codebase.
backend/analytics_server/mhq/service/settings/models.py (2)
49-53: Implementation looks good for the IncidentPRFilter dataclass.The dataclass is well-designed with a clear purpose - to define key-value pairs for PR filtering where:
fieldspecifies which PR attribute to match againstvaluecontains the regex pattern to use for matchingThis simple structure will be flexible enough to support filtering against various PR attributes.
56-58: Well-structured IncidentPRsSetting class with appropriate properties.This setting class provides:
- A boolean flag
include_revert_prsto control the inclusion of revert PRs in incident reports- A list of filter criteria via the
filterspropertyThis implementation satisfies the PR objectives by creating a setting that allows for regex-based filtering of incident PRs.
backend/analytics_server/mhq/service/incidents/incident_filter.py (3)
7-7: Import statement correctly updated for the new setting class.The import statement has been updated to include
IncidentPRsSetting, which is used in the modified code below.
10-10: Good practice importing the specific enum needed.The import statement for
IncidentTypeis correctly added to support the comparison operation withIncidentType.REVERT_PR.
112-125: The implementation correctly handles excluding REVERT_PR incidents.This code effectively:
- Checks if
INCIDENT_PRS_SETTINGis present in the setting types- Retrieves the corresponding setting
- When
include_revert_prsis set toFalse, filters outREVERT_PRfrom incident typesThis satisfies the PR objective to "add an option to exclude REVERT_PR from incident types when the Incident PRs setting has the
include_revert_prsflag set to false."One small optimization could be to early return if
incident_typesis empty, but the current implementation is fine.backend/analytics_server/mhq/service/code/pr_filter.py (3)
4-4: The import statement correctly includes the new setting class.Importing
IncidentPRsSettingalongsideExcludedPRsSettingis appropriate as both are used in this file.
106-107: Good implementation of the condition for applying incident PR filters.This code follows the same pattern as the existing condition for excluded PR IDs, making it consistent with the codebase's style.
117-120: The method correctly applies incident PR filters.This implementation:
- Initializes
incident_pr_filtersas an empty list if it's None- Appends the filters from the settings object
The approach matches the pattern used in
_apply_excluded_pr_ids_setting, maintaining code consistency.backend/analytics_server/mhq/store/models/code/filter.py (2)
16-16: New attribute correctly added to the PRFilter dataclass.The
incident_pr_filtersattribute is appropriately typed asList[Dict]and set as optional with a default value of None.
102-102: Correctly added the new query to the conditions dictionary.The incident PR filters query is properly integrated into the existing conditions dictionary.
backend/analytics_server/mhq/api/resources/settings_resource.py (2)
8-8: Good addition of the IncidentPRsSetting import.This import aligns with the PR objective to implement settings for Incident PRs and is used appropriately in the code below.
59-63: Implementation looks good.The code correctly adds support for the new
IncidentPRsSettingtype in the response, exposing both theinclude_revert_prsflag andfiltersfields. This approach is consistent with how other setting types are handled in this module.backend/analytics_server/mhq/service/incidents/models/adapter.py (1)
6-24: Well-implemented adapter pattern.The
IncidentPRAdapterfollows a clean design pattern with a static method that transforms PR data into an incident model. The implementation correctly maps all necessary fields from both the incident PR and resolution PR.Two observations:
- The adapter ensures the incident type is set to
IncidentType.REVERT_PR, which aligns with the PR objectives- The implementation properly sets the status to resolved and uses appropriate timestamps from the PRs
backend/analytics_server/tests/service/Incidents/test_incident_types_setting.py (2)
12-38: Test for incident types settings looks good.This test verifies that when only
INCIDENT_TYPES_SETTINGis present, all incident types are properly included in the filter. The test is comprehensive and includes all incident types.
41-55: Edge case test for empty incident types is well implemented.Good test case that verifies behavior when the incident types setting exists but contains an empty list. This is an important edge case to cover.
backend/analytics_server/mhq/api/incidents.py (7)
39-39: Good addition of the optional pr_filter parameter.The
pr_filterparameter is correctly defined as optional in both the schema and function signature. The type conversion from JSON string to dictionary is handled appropriately.Also applies to: 44-45
51-53: Filter propagation well implemented.The code correctly processes the PR filter through
apply_pr_filterand passes it to the incident service method. This ensures consistent filtering behavior across all endpoints.Also applies to: 58-58
100-102: PR filter properly integrated with get_team_incidents.The implementation correctly passes the PR filter to the incident service's
get_team_incidentsmethod, aligning with the PR objectives for improved incident filtering.
124-124: Consistent implementation for MTTR endpoint.The code follows the same pattern as other endpoints, adding the optional PR filter parameter and properly processing it for the MTTR calculations.
Also applies to: 129-130, 135-137
154-154: Consistent implementation for MTTR trends endpoint.The code follows the same pattern as other endpoints, adding the optional PR filter parameter and properly processing it for the MTTR trend calculations.
Also applies to: 159-160, 165-167
220-222: PR filter properly integrated with CFR calculations.The implementation correctly passes the PR filter to the incident service's method for fetching incidents used in CFR calculations.
266-268: PR filter properly integrated with CFR trends calculations.The implementation correctly passes the PR filter to the incident service's method for fetching incidents used in CFR trend calculations.
backend/analytics_server/tests/service/Incidents/test_incident_types_setting.py
Outdated
Show resolved
Hide resolved
backend/analytics_server/tests/service/Incidents/test_incident_types_setting.py
Outdated
Show resolved
Hide resolved
backend/analytics_server/tests/service/Incidents/test_incident_types_setting.py
Outdated
Show resolved
Hide resolved
backend/analytics_server/tests/service/Incidents/test_incident_types_setting.py
Outdated
Show resolved
Hide resolved
backend/analytics_server/mhq/service/settings/configuration_settings.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
backend/analytics_server/mhq/service/incidents/incidents.py (4)
47-67:⚠️ Potential issueFix potential serialization issue with dict_values view
The method returns a dict_values view rather than a concrete list, which could cause issues with JSON serialization or when callers expect list semantics like indexing.
Apply this fix:
- return {incident.key: incident for incident in total_incidents}.values() + return list({incident.key: incident for incident in total_incidents}.values())
69-91:⚠️ Potential issueFix potential serialization issue with dict_values view
The method returns a dict_values view rather than a concrete list, which could cause issues with JSON serialization or when callers expect list semantics like indexing.
Apply this fix:
- return {incident.key: incident for incident in total_incidents}.values() + return list({incident.key: incident for incident in total_incidents}.values())
97-105:⚠️ Potential issueFix potential AttributeError when setting is absent
The code calls
get_settings()which can returnNone, but then accesses.specific_settingsunconditionally, risking an AttributeError for teams that have never saved this setting.Apply this fix:
- incident_prs_setting: IncidentPRsSetting = self._settings_service.get_settings( + setting = self._settings_service.get_settings( setting_type=SettingType.INCIDENT_PRS_SETTING, entity_type=EntityType.TEAM, entity_id=team_id, - ).specific_settings + ) + incident_prs_setting: IncidentPRsSetting = setting.specific_settings if setting else None
113-118:⚠️ Potential issueFix TypeError with asdict and avoid parameter shadowing
There are two issues in this code:
asdict()only works on dataclass instances, butPRFilteris not a dataclass- Reusing the variable name
pr_filtershadows the input parameterApply this fix:
- resolution_prs_filter: PRFilter = apply_pr_filter( - asdict(pr_filter), + processed_pr_filter: PRFilter = apply_pr_filter( + vars(pr_filter), # or another appropriate dict conversion method EntityType.TEAM, team_id, [SettingType.EXCLUDED_PRS_SETTING, SettingType.INCIDENT_PRS_SETTING], ) resolution_prs = self._code_repo_service.get_prs_merged_in_interval( - team_repo_ids, resolution_prs_interval, resolution_prs_filter + team_repo_ids, resolution_prs_interval, processed_pr_filter )
🧹 Nitpick comments (1)
backend/analytics_server/mhq/service/incidents/incidents.py (1)
126-142: Consider adding logging and optimizing nested loopsThe nested loops in this section could become inefficient with a large number of PRs and filters. Consider adding debug logging to help diagnose issues and optimize the structure.
+ # Map to track PRs that have already matched a filter + matched_prs = set() for pr in resolution_prs: + if pr.id in matched_prs: + continue + for filter in incident_prs_setting.filters: incident_pr_number = self._extract_pr_number_from_regex( getattr(pr, filter["field"]), filter["value"] ) if incident_pr_number: + matched_prs.add(pr.id) pr_numbers.append(incident_pr_number) if str(pr.repo_id) not in repo_id_to_pr_number_to_pr_map: repo_id_to_pr_number_to_pr_map[str(pr.repo_id)] = {} repo_id_to_pr_number_to_pr_map[str(pr.repo_id)][ incident_pr_number ] = pr break
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
backend/analytics_server/mhq/service/incidents/incidents.py(4 hunks)backend/analytics_server/mhq/store/repos/code.py(1 hunks)backend/analytics_server/tests/service/Incidents/test_team_pr_incidents.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/analytics_server/mhq/store/repos/code.py (4)
backend/analytics_server/tests/service/Incidents/test_team_pr_incidents.py (1)
get_prs_merged_in_interval_by_numbers(78-79)backend/analytics_server/mhq/utils/time.py (1)
Interval(14-96)backend/analytics_server/mhq/store/models/code/filter.py (1)
PRFilter(10-108)backend/analytics_server/mhq/store/models/code/pull_requests.py (1)
PullRequest(12-76)
backend/analytics_server/mhq/service/incidents/incidents.py (10)
backend/analytics_server/mhq/service/settings/models.py (1)
IncidentPRsSetting(56-58)backend/analytics_server/mhq/store/models/code/filter.py (1)
PRFilter(10-108)backend/analytics_server/mhq/store/repos/code.py (3)
get_active_team_repos_by_team_id(263-268)get_prs_merged_in_interval(287-308)get_prs_merged_in_interval_by_numbers(311-327)backend/analytics_server/mhq/utils/time.py (4)
time_now(10-11)Interval(14-96)from_time(26-27)to_time(30-31)backend/analytics_server/mhq/service/settings/configuration_settings.py (2)
get_settings_service(493-494)get_settings(119-131)backend/analytics_server/mhq/store/repos/incidents.py (1)
IncidentsRepoService(23-211)backend/analytics_server/mhq/service/incidents/models/adapter.py (2)
IncidentPRAdapter(6-24)adapt(8-24)backend/analytics_server/mhq/service/code/pr_filter.py (1)
apply_pr_filter(11-34)backend/analytics_server/mhq/service/incidents/incident_filter.py (1)
apply_incident_filter(45-62)backend/analytics_server/tests/service/Incidents/test_team_pr_incidents.py (4)
get_settings(30-50)get_active_team_repos_by_team_id(63-73)get_prs_merged_in_interval(75-76)get_prs_merged_in_interval_by_numbers(78-79)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: All file linting
🔇 Additional comments (13)
backend/analytics_server/mhq/store/repos/code.py (1)
310-327: Well-implemented repository method for PR filtering by numbersThe implementation follows the established pattern of other query methods in this service, with appropriate deferred loading of the data field, filtering by repos, interval, PR filter, and PR numbers, and proper ordering of results.
backend/analytics_server/tests/service/Incidents/test_team_pr_incidents.py (8)
16-19: LGTM: Simple and effective test interval setupThe test interval is set appropriately with realistic time values for testing.
22-26: LGTM: Good use of fixture with patchingThe mock_apply_pr_filter fixture effectively isolates the tests from the actual filter application logic.
29-55: LGTM: Well-structured fake settings serviceThe fake service provides realistic test data with appropriate regex patterns for extracting PR numbers from branches and titles.
58-79: LGTM: Complete fake repository service implementationThe implementation properly handles both filtering methods for testing different PR retrieval approaches.
86-101: LGTM: Good edge case test for empty filtersThis test verifies that the service returns an empty list when there are no incident PR filters.
104-151: LGTM: Comprehensive test for PR filteringThe test thoroughly checks that the correct PRs are identified as incidents based on the revert patterns.
154-187: LGTM: Good test for multiple repos edge caseThis test verifies behavior when PRs match the filter pattern but no original PRs are found.
190-236: LGTM: Thorough test for the complete multi-repo scenarioThis test effectively verifies that the service can identify and return incidents from multiple repositories.
backend/analytics_server/mhq/service/incidents/incidents.py (4)
3-8: LGTM: Well-organized importsThe imports are properly organized by category (typing, core services, models, utilities) which improves code readability.
Also applies to: 22-34
37-45: LGTM: Proper constructor extensionGood addition of the CodeRepoService dependency to the constructor, following the dependency injection pattern.
160-169: LGTM: Well-implemented regex extraction methodThe method properly validates both inputs, checks the regex pattern, and safely extracts the first capture group. Good defensive programming.
331-334: LGTM: Updated factory method with new dependencyThe factory method has been properly updated to include the new CodeRepoService dependency.
Kamlesh72
force-pushed
the
feat/pr-filtering-backend
branch
from
5e32205 to
9536dd5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/analytics_server/mhq/service/incidents/incidents.py (1)
118-123:⚠️ Potential issueFix the use of
asdict()on a non-dataclass instanceThe
PRFilterclass is not a dataclass, so callingasdict(pr_filter)will raise aTypeError. This was mentioned in a previous review.- resolution_prs_filter: PRFilter = apply_pr_filter( - asdict(pr_filter), + resolution_prs_filter: PRFilter = apply_pr_filter( + vars(pr_filter) if pr_filter else None, EntityType.TEAM, team_id, [SettingType.EXCLUDED_PRS_SETTING, SettingType.INCIDENT_PRS_SETTING], )
🧹 Nitpick comments (1)
backend/analytics_server/mhq/service/incidents/incidents.py (1)
116-117: Consider limiting the time range for resolution PRsUsing
time_now()forto_timein the resolution interval could result in scanning an unnecessarily large time range if the original interval is far in the past.Consider adding a reasonable upper bound to the interval, perhaps a few months beyond the original interval's end time.
- resolution_prs_interval = Interval( - from_time=interval.from_time, to_time=time_now() - ) + # Limit the resolution interval to a reasonable range (e.g., 3 months after interval end) + max_resolution_time = max(interval.to_time, time_now()) + resolution_prs_interval = Interval( + from_time=interval.from_time, to_time=max_resolution_time + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
backend/analytics_server/mhq/service/incidents/incidents.py(4 hunks)backend/analytics_server/mhq/store/repos/code.py(1 hunks)backend/analytics_server/tests/service/Incidents/test_team_pr_incidents.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/analytics_server/mhq/store/repos/code.py
- backend/analytics_server/tests/service/Incidents/test_team_pr_incidents.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/analytics_server/mhq/service/incidents/incidents.py (10)
backend/analytics_server/mhq/service/settings/models.py (1)
IncidentPRsSetting(56-58)backend/analytics_server/mhq/store/models/code/filter.py (1)
PRFilter(10-108)backend/analytics_server/mhq/store/repos/code.py (4)
CodeRepoService(27-513)get_active_team_repos_by_team_id(263-268)get_prs_merged_in_interval(287-308)get_prs_merged_in_interval_by_numbers(311-327)backend/analytics_server/mhq/utils/time.py (2)
time_now(10-11)Interval(14-96)backend/analytics_server/mhq/service/settings/configuration_settings.py (3)
SettingsService(22-490)get_settings_service(493-494)get_settings(119-131)backend/analytics_server/mhq/service/incidents/models/adapter.py (2)
IncidentPRAdapter(6-24)adapt(8-24)backend/analytics_server/mhq/service/code/pr_filter.py (1)
apply_pr_filter(11-34)backend/analytics_server/mhq/service/incidents/incident_filter.py (1)
apply_incident_filter(45-62)backend/analytics_server/mhq/store/models/settings/configuration_settings.py (1)
SettingType(14-22)backend/analytics_server/tests/service/Incidents/test_team_pr_incidents.py (4)
get_settings(30-50)get_active_team_repos_by_team_id(63-73)get_prs_merged_in_interval(75-76)get_prs_merged_in_interval_by_numbers(78-79)
🔇 Additional comments (6)
backend/analytics_server/mhq/service/incidents/incidents.py (6)
93-163: **Well-implemented PR incidents retrieval! **The new
get_team_pr_incidentsmethod effectively implements the core functionality described in the PR objectives. It appropriately fetches incident PRs based on regex filters defined in the settings, processes them, and adapts them into incident objects.
103-106: Good null-check implementationI appreciate the defensive coding here, properly checking if settings exist before trying to access specific_settings.
67-67: Thanks for addressing the previous feedbackGood job converting the dict_values view to a list, as recommended in the previous review comments. This will prevent potential issues with callers expecting list semantics.
91-91: Consistent implementation of the dict_values fixThe same fix has been properly applied here as well.
165-174: Well-implemented regex extraction helperThe
_extract_pr_number_from_regexmethod is cleanly implemented with proper error handling and validations.
336-339: Factory function properly updatedThe
get_incident_servicefunction has been correctly updated to pass theCodeRepoServiceto theIncidentServiceconstructor.
| } | ||
| ), | ||
| ) | ||
| def get_resolved_incidents(team_id: str, from_time: datetime, to_time: datetime): | ||
| def get_resolved_incidents( | ||
| team_id: str, from_time: datetime, to_time: datetime, pr_filter: dict = None |
There was a problem hiding this comment.
| team_id: str, from_time: datetime, to_time: datetime, pr_filter: dict = None | |
| team_id: str, from_time: datetime, to_time: datetime, pr_filter: typeOptional[dict] = None |
Type of Dict cannot be assigned to None. Use correct type. Import the correct optional from typing and ensure it is not conflicting with voluptuous.Optional
| class IncidentPRAdapter: | ||
| @staticmethod | ||
| def adapt(incident_pr: PullRequest, resolution_pr: PullRequest) -> Incident: | ||
| return Incident( | ||
| id=incident_pr.id, | ||
| provider=incident_pr.provider, | ||
| key=str(incident_pr.id), | ||
| title=incident_pr.title, | ||
| incident_number=int(incident_pr.number), | ||
| status=IncidentStatus.RESOLVED.value, | ||
| creation_date=incident_pr.state_changed_at, | ||
| acknowledged_date=resolution_pr.created_at, | ||
| resolved_date=resolution_pr.state_changed_at, | ||
| assigned_to=resolution_pr.author, | ||
| assignees=[resolution_pr.author], | ||
| url=incident_pr.url, | ||
| meta={}, | ||
| incident_type=IncidentType.REVERT_PR, | ||
| ) |
There was a problem hiding this comment.
Why is this a class? Can it not be simply a function?
| resolution_prs_filter: PRFilter = apply_pr_filter( | ||
| asdict(pr_filter), | ||
| EntityType.TEAM, | ||
| team_id, | ||
| [SettingType.EXCLUDED_PRS_SETTING, SettingType.INCIDENT_PRS_SETTING], | ||
| ) |
There was a problem hiding this comment.
nit: Use named arguments when there are multiple parameters accepted by the function. Helps avoid any misplaced arguments, which become super difficult to debug later on. (Speaking from experience 🥲)
| pr_numbers.append(incident_pr_number) | ||
| if str(pr.repo_id) not in repo_id_to_pr_number_to_pr_map: | ||
| repo_id_to_pr_number_to_pr_map[str(pr.repo_id)] = {} | ||
| repo_id_to_pr_number_to_pr_map[str(pr.repo_id)][ | ||
| incident_pr_number | ||
| ] = pr | ||
| break |
There was a problem hiding this comment.
Can be made cleaner using defaultdicts(dict). Then you won't have to the extra conditional check to see if str(pr.repo_id) not in repo_id_to_pr_number_to_pr_map.
| for pr in resolution_prs: | ||
| for filter in incident_prs_setting.filters: | ||
| incident_pr_number = self._extract_pr_number_from_regex( | ||
| getattr(pr, filter["field"]), filter["value"] |
There was a problem hiding this comment.
I don't get this. If filter is a dataclass how are you accessing the property using __getitem__. This would fail no?
There was a problem hiding this comment.
is filter a json object or a dataclass?
There was a problem hiding this comment.
From what i can see from the rest of the code, filters is a json object and not a dataclass. I think you wanted to define the structure of the filters, but ended up using the wrong tool for the job. You can use typedDicts in python to define the structure of the python dicts. They work like typescript's interface.
There was a problem hiding this comment.
@adnanhashmi09 I defined filter as dataclass but wasn't converting it from dict to dataclass. That's why maybe no error as it is still a dictionary.
I will handle through frontend that field inside filter is either title or head_branch as in UI it will be displayed as List of Options.
On backend side we will rely only on type checking. works?
class IncidentPRFilter(TypedDict):
field: Literal["title", "head_branch"]
value: strThere was a problem hiding this comment.
i am not sure what you mean. If you want type safety using dataclasses then you need to adapt the json filter adapter to a dataclass first and then access the properties using the dot notation.
but if you don't want to go that route and have the filters remain as dicts then it would be nice to have them use TypedDicts if you want the syntactic sugar of having the types. Do remember, that typedDicts don't provide runtime typesafety. TypedDicts just help your intellisense work well.
|
|
||
| def test_get_team_pr_incidents_with_multiple_repos_and_incidents(): | ||
|
|
||
| prs_using_filters = [ |
There was a problem hiding this comment.
| prs_using_filters = [ | |
| resolution_prs = [ |
can be named better to signify what exactly those prs are
similar for other variables in the test
There was a problem hiding this comment.
My bad. This should have been named resolution_prs as they are fetched using our PR Incident filters.
There was a problem hiding this comment.
Also I shouldn't rename prs_using_numbers right ?
Because this PRs are Incident PRs is not confirmed yet.
Ex: Resolution PR revert 123 and we have 3 Repos.
Then we try to fetch PR number 123 from each Repo.
So len(prs_using_numbers) = 3 but only one of these will be Incident PR.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
backend/analytics_server/mhq/service/incidents/incidents.py (3)
134-151: Optimize with defaultdict to eliminate conditional checks.As suggested in past review comments, using
defaultdict(dict)eliminates the need for the conditional check when accessing the nested dictionary.The current implementation is already using
defaultdict(dict)correctly at line 136-138, so this optimization is already in place.
118-126:⚠️ Potential issueCritical: Fix the
asdictusage on non-dataclass.This is the same issue flagged in past review comments.
PRFilteris not a dataclass, soasdict(pr_filter)will raise aTypeError. Also, the variable name shadows the input parameter.- resolution_prs_filter: PRFilter = apply_pr_filter( - pr_filter=asdict(pr_filter), + resolution_prs_filter: PRFilter = apply_pr_filter( + pr_filter=vars(pr_filter) if pr_filter else None, entity_type=EntityType.TEAM, entity_id=team_id, setting_types=[ SettingType.EXCLUDED_PRS_SETTING, SettingType.INCIDENT_PRS_SETTING, ], )
140-151:⚠️ Potential issueCritical: Add field validation to prevent AttributeError.
This is the same issue flagged in past review comments. The code could raise an
AttributeErroriffilter["field"]is not a valid attribute of the PR object.incident_pr_number = self._extract_pr_number_from_regex( - getattr(pr, filter["field"]), filter["value"] + getattr(pr, filter["field"], "") if hasattr(pr, filter["field"]) else "", + filter["value"] )
🧹 Nitpick comments (1)
backend/analytics_server/mhq/service/incidents/incidents.py (1)
33-33: Remove unused import.The
asdictimport fromdataclassesis imported but not properly used in the code. Based on past review comments, there's an issue with its usage on line 119 where it's called on aPRFilterinstance that isn't a dataclass.-from dataclasses import asdict
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/analytics_server/mhq/api/incidents.py(11 hunks)backend/analytics_server/mhq/service/incidents/incidents.py(4 hunks)backend/analytics_server/mhq/service/incidents/models/adapter.py(1 hunks)backend/analytics_server/mhq/service/settings/models.py(2 hunks)backend/analytics_server/mhq/store/models/code/filter.py(3 hunks)backend/analytics_server/tests/service/Incidents/test_team_pr_incidents.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- backend/analytics_server/mhq/service/settings/models.py
- backend/analytics_server/mhq/store/models/code/filter.py
- backend/analytics_server/tests/service/Incidents/test_team_pr_incidents.py
- backend/analytics_server/mhq/service/incidents/models/adapter.py
- backend/analytics_server/mhq/api/incidents.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/analytics_server/mhq/service/incidents/incidents.py (10)
backend/analytics_server/mhq/service/settings/models.py (1)
IncidentPRsSetting(55-57)backend/analytics_server/mhq/store/models/code/filter.py (1)
PRFilter(10-108)backend/analytics_server/mhq/store/repos/code.py (3)
get_active_team_repos_by_team_id(263-268)get_prs_merged_in_interval(287-308)get_prs_merged_in_interval_by_numbers(311-327)backend/analytics_server/mhq/utils/time.py (2)
time_now(10-11)Interval(14-96)backend/analytics_server/mhq/service/settings/configuration_settings.py (3)
SettingsService(22-490)get_settings_service(493-494)get_settings(119-131)backend/analytics_server/mhq/service/incidents/models/adapter.py (1)
adaptIncidentPR(6-22)backend/analytics_server/mhq/service/code/pr_filter.py (1)
apply_pr_filter(11-34)backend/analytics_server/mhq/service/incidents/incident_filter.py (1)
apply_incident_filter(45-62)backend/analytics_server/mhq/store/models/settings/configuration_settings.py (1)
SettingType(14-22)backend/analytics_server/tests/service/Incidents/test_team_pr_incidents.py (4)
get_settings(30-50)get_active_team_repos_by_team_id(63-73)get_prs_merged_in_interval(75-76)get_prs_merged_in_interval_by_numbers(78-79)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: All file linting
🔇 Additional comments (8)
backend/analytics_server/mhq/service/incidents/incidents.py (8)
3-8: LGTM! Imports are well-organized and necessary.The new imports support the PR incident functionality correctly. All imports appear to be used appropriately within the code.
41-45: LGTM! Constructor properly updated with new dependency.The addition of
CodeRepoServicedependency is correctly implemented and stored as an instance variable for use in the new PR incident functionality.
48-67: LGTM! Method correctly enhanced to include PR incidents.The method properly combines traditional incidents with PR-based incidents, sorts by creation date, and deduplicates by incident key. The implementation addresses the previous issue of returning a proper list instead of dict_values.
69-91: LGTM! Consistent implementation with resolved incidents method.The method follows the same pattern as
get_resolved_team_incidents, correctly combining and deduplicating incidents from both sources.
153-167: LGTM! PR filtering and incident adaptation logic is sound.The code correctly:
- Queries PRs by numbers within the interval
- Applies the PR filter
- Maps PRs to their resolution PRs
- Adapts them to incidents using the adapter function
170-179: LGTM! Safe regex extraction with proper validation.The helper method correctly:
- Validates input parameters
- Uses the regex validation utility
- Safely extracts the first capture group
- Returns None for invalid patterns or no matches
219-239: LGTM! MTTR methods correctly updated to accept PR filter.Both MTTR methods properly pass the
pr_filterparameter to the underlying incident retrieval methods, maintaining consistency across the API.
342-344: LGTM! Factory function correctly updated with new dependency.The factory function properly instantiates
IncidentServicewith all required dependencies including the newCodeRepoService.
Linked Issue(s)
Ref #557
Acceptance Criteria fulfillment
include_revert_prsto falseSummary by CodeRabbit
New Features
Tests
Chores