feat(perf-detectors): Complete PERFORMANCE_DETECTOR_CONFIG_MAPPINGS#109624
feat(perf-detectors): Complete PERFORMANCE_DETECTOR_CONFIG_MAPPINGS#109624
Conversation
github-actions
bot
added
the
Scope: Backend
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| ), | ||
| DetectorType.RENDER_BLOCKING_ASSET_SPAN: PerformanceDetectorConfigMapping( | ||
| settings_key=DetectorType.RENDER_BLOCKING_ASSET_SPAN, | ||
| wfe_detector_type="performance_render_blocking_asset_span", |
There was a problem hiding this comment.
Could we use the group type slug here to avoid hard coding, or is this the thing you were referring to earlier where you don't want to tie to the group type?
There was a problem hiding this comment.
This is what I'm currently churning on.
I have some docs written up for Monday discussion, but the fun case here is QueryInjectionVulnerabilityGroupType, which is actually associated with two different Performance Detectors that look for separate things. The 1:1 mapping from Detector to GroupType isn't strictly necessary (it's mostly weird when we want to say which Detector created a group if multiple are involved), and logically it seems true that there can be multiple ways to generate a particular type of Group.
That said, these are strings also for mundane reasons: importing every involved GroupType makes me feel bad if I only really need them because I want to name the Detector that happens to be named after them.
I added a test to validate that all of these are registered group types to make that safer, but maybe I should give in and just use them by name. I don't think it'll actually be a problem.
| for field_name in mapping.option_keys: | ||
| assert field_name in detector_settings, ( | ||
| f"option_keys field '{field_name}' for DetectorType.{detector_type.name} " | ||
| f"not found in get_detection_settings()[{detector_type}]" | ||
| ) | ||
|
|
||
| # Every option key value must be a key in get_merged_settings | ||
| for field_name, option_key in mapping.option_keys.items(): |
There was a problem hiding this comment.
Could you combine these 2 loops?
2 participants
A partial mapping is fine, but a complete one forces us to acknowledge all of the corner cases, and allows us to ensure no new one sneak in.
Fixes ISWF-2155.