-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(detectors): Add "make new detector" script #97429
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
base: master
Are you sure you want to change the base?
Conversation
scripts/create_detector.py
Outdated
class MonitorIncidentType(GroupType): | ||
type_id = None # TODO |
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.
Potential bug: The `create_detector.py` script hardcodes the `MonitorIncidentType` class name, which will cause duplicate class definition errors and crash the server on import.
- Description: The
create_detector.py
script appends a hardcodedGroupType
class definition tosrc/sentry/issues/grouptype.py
. The class is always namedMonitorIncidentType
. Since this class already exists in the target file, running the script will create a duplicate class definition. This results in a Python syntax error whengrouptype.py
is imported, which will crash any Sentry service that depends on this module, preventing the application from starting. - Suggested fix: Generate the
GroupType
class name dynamically based on thedetector_name
input, similar to howdetector_classname
is created. A new variable, likegrouptype_classname
, should be created fromdetector_name
and used within the template string written togrouptype.py
.
severity: 0.9, confidence: 0.95
Did we get this right? 👍 / 👎 to inform future reviews.
scripts/create_detector.py
Outdated
@@ -0,0 +1,126 @@ | |||
""" |
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.
the scripts directory is meant to be tools for the build system and development environment
admittedly one off scripts like this are sort of frowned upon because they almost instantly become outdated, unused, untested, and unmaintained
it may make sense to make a separate repository where you can adequately test this and maintain it?
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.
the scripts directory is meant to be tools for the build system and development environment
Moved out of the scripts
directory and to the tools
directory.
admittedly one off scripts like this are sort of frowned upon because they almost instantly become outdated, unused, untested, and unmaintained
Generally fair.
- I can push back a little on "unused" — this would be pretty heavily linked from our documentation (replacing half of that 25-step checklist), so it should be fairly discoverable.
- Untested, well, I could do a snapshot test to ensure that any change to outputs would be noted, but that feels like cheating to me (basically just "testing" that it works in its current state and doesn't change from current state).
- No argument on "outdated" + "unmaintained", not sure what I can say there except that I would be fairly willing to delete this if it stops working (since once it's committed it's easy to find in version history).
it may make sense to make a separate repository where you can adequately test this and maintain it?
The goal of this script is "make detector-creation as close to zero friction as possible"; having to check out a new repo and run a script cross-repo is more friction than I'd like.
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.
tools is also a build system / development environment namespace too 😆
let's take a step back -- if it's hard to add one of these things why not make the actual steps to adding them simpler rather than adding a hacky script?
a8d2f89
to
a104b29
Compare
performance_detection_file_contents, | ||
) | ||
performance_detection_file_contents = sub( | ||
r"(def get_detection_settings[\(\):.\w\s=\|\[\]\->_,]+return \{)(([.\s]*DetectorType\.[^\{]+[^\}]+\},?)+)(\s+\})", |
Check failure
Code scanning / CodeQL
Inefficient regular expression High
a104b29
to
c2452c3
Compare
Summary
Adding a new detector is a very involved process.
This PR adds a helper script to do as much work in an automated manner as possible to make it easier to add new detectors.
Test Plan
Ran
python3 scripts/create_detector.py solar_eclipse
and observed that modified files looked about right.