-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(aci): Add monitor created/updated analytics #103279
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
Adds some basic tracking of type, dataset, aggregate. I think a lot of other fields are already normalized in the database and can be queried at any time.
| const snubaQuery: SnubaQuery | undefined = | ||
| resultDetector.type === 'metric_issue' | ||
| ? resultDetector.dataSources[0]?.queryObj?.snubaQuery | ||
| : undefined; |
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.
Bug: Unsafe access to resultDetector.dataSources[0] can cause a TypeError if dataSources is null from backend.
Severity: HIGH | Confidence: 0.95
🔍 Detailed Analysis
The frontend code in useCreateDetectorFormSubmit.tsx (lines 49-52) attempts to access resultDetector.dataSources[0] without checking if resultDetector.dataSources is null. The backend serializer in detector_serializer.py can return None for data_sources if no DataSourceDetector relationships exist, leading to a TypeError: "Cannot read property '0' of null". This type mismatch between backend serialization and frontend TypeScript definition causes a runtime crash, preventing analytics events from being recorded and potentially disrupting the UI flow after detector creation/update.
💡 Suggested Fix
Apply optional chaining to the array index access: resultDetector.dataSources?.[0]?.queryObj?.snubaQuery to safely handle null or empty dataSources.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: static/app/views/detectors/hooks/useCreateDetectorFormSubmit.tsx#L49-L52
Potential issue: The frontend code in `useCreateDetectorFormSubmit.tsx` (lines 49-52)
attempts to access `resultDetector.dataSources[0]` without checking if
`resultDetector.dataSources` is `null`. The backend serializer in
`detector_serializer.py` can return `None` for `data_sources` if no `DataSourceDetector`
relationships exist, leading to a `TypeError: "Cannot read property '0' of null"`. This
type mismatch between backend serialization and frontend TypeScript definition causes a
runtime crash, preventing analytics events from being recorded and potentially
disrupting the UI flow after detector creation/update.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference_id: 2634920
| platform: string; | ||
| }; | ||
| 'monitor.created': { | ||
| detector_type: |
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.
Can you reuse DetectorType or just make this string? Might be annoying to have to update when we add new ones
| trackAnalytics('monitor.created', { | ||
| organization, | ||
| detector_type: resultDetector.type, | ||
| dataset: snubaQuery?.dataset, |
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.
Hmm this also might be annoying to deal with as we add more types. Maybe we can add some function on the config which would take a detector and return the properties that it should record for analytics?
Adds some basic tracking of type, dataset, aggregate. I think a lot of other fields are already normalized in the database and can be queried at any time.