ref(detectors): Add abstract-ish base class for DetectorHandler #108000
Conversation
github-actions
bot
added
the
Scope: Backend
saponifi3d
left a comment
saponifi3d
left a comment
There was a problem hiding this comment.
No blocking comments here really; agree with most of the changes and just thoughts about the future.
| def __init__(self, detector: Detector): | ||
| self.detector = detector |
There was a problem hiding this comment.
Should we keep this as a pure abc and only have type definitions?
There was a problem hiding this comment.
I wanted to, but an abstract constructor felt strange when an actual constructor gave us what we needed. DetectorSettings could use a Callable which constructs the correct type, could've gone with an abstract static constructor, but in nearly all worlds it seemed like we'd just be requiring someone to implement this init, so I went with it. Open to alternatives, but wasn't sold by the ones I considered as being better.
| @@ -14,7 +14,7 @@ | |||
| from sentry.testutils.silo import region_silo_test | |||
| from sentry.uptime.grouptype import UptimeDomainCheckFailure | |||
| from sentry.workflow_engine.handlers.detector import ( | |||
| DetectorHandler, | |||
| BaseDetectorHandler, | |||
There was a problem hiding this comment.
for future - this could be a super cool abstraction; we could create a TestDetectorHandler and all the registration info. right now most of the tests are loosely based on an existing detector type cause... time.
| return GroupedDetectorEvaluationResult(result={}, tainted=False) | ||
| def evaluate( | ||
| self, data_packet: DataPacket[object] | ||
| ) -> dict[DetectorGroupKey, DetectorEvaluationResult]: |
There was a problem hiding this comment.
One thing i've struggled with in these abstractions is the DetectorGroupKey - and grouping in general for the results.. 🤔 should we have a grouping detector that handles all of this?
I haven't bottomed out on any of that stuff, just a thought i've been struggling with when looking at the base class. (the functional programmer in me wants to make GroupingDetector and decorate these classes with it)
There was a problem hiding this comment.
I've been spinning on this a bit myself, and I'm not sure yet. This change is partially a spinoff of me trying to reason through these issues.
| # TODO - @saponifi3d - Change this class to be a pure ABC and remove the `__init__` method. | ||
| # TODO - @saponifi3d - Once the change is made, we should introduce a `BaseDetector` class to evaluate simple cases | ||
| class DetectorHandler(abc.ABC, Generic[DataPacketType, DataPacketEvaluationType]): | ||
| class DetectorHandler(abc.ABC, Generic[DataPacketType]): |
There was a problem hiding this comment.
if this is kept as a pure ABC, should it live in workflow_engine/types?
2 participants
DetectorHandlers are simple from the outside, and this aims to shift us to a base interface that reflects that, pushing the internal subclass interface into BaseDetectorHandler, which can (eventually) become our default implementation base for stateless Detectors.
The main observable fix here is that DetectorHandler loses a type parameter that wasn't visible to callers, simplifying and possibly opening the door to type-based runtime validation.