feat: Initial monitors implementation #11602

dcramer · 2019-01-18T01:41:19Z

Add support for schedule-based push monitors
And endpoint for creating and updating checkins (DSN-based auth supported)

dcramer · 2019-01-18T01:42:14Z

If anyone has thoughts on the current implementation its almost in a usable state. The main missing components are:

we need to generate an event (this is actually complicated to do right now)
we'll likely add a new event type for monitors ("monitor failure")
we need to make sure rules expose event type so we can define one specifically for monitors
theres absolutely no UI, but thats ok for now

markstory · 2019-01-21T15:28:26Z

src/sentry/api/endpoints/monitor_checkins.py

+
+    def post(self, request, project, monitor):
+        """
+        Create a new check-in for a monitor


Are you thinking that users would make an API request directly, or would monitor creation be part of the SDKs?

it will happen via the UI or API

markstory · 2019-01-21T15:30:53Z

src/sentry/api/endpoints/monitor_checkins.py

+                organization_id=project.organization_id,
+                monitor_id=monitor.id,
+                duration=result.get('duration'),
+                status=getattr(CheckInStatus, result['status']),


Do monitors need to have release or environment context? Having releases/environments would allow users to track when jobs broke around deploys and whether or not they are just missing in staging.

Release won't work for this kind of thing. We could probably allow environment.

markstory · 2019-01-21T15:32:50Z

src/sentry/models/monitor.py

+        default=MonitorType.UNKNOWN,
+        choices=MonitorType.as_choices(),
+    )
+    config = EncryptedJsonField(default=dict)


What goes in here? Could any of those fields be part of the official schema?

schedule is the only one, but we'd have to agree to always store it the same for any kind of monitor

also there is a spec for this if you want to read (its in notion)

markstory · 2019-01-21T15:38:42Z

src/sentry/models/monitor.py

+
+
+def generate_secret():
+    return uuid4().hex + uuid4().hex


You can get this function from sentry.models.apitoken:generate_token

the way django 1.8+ generates migration the default has to be module-scoped, which is the only reason im not using a lambda here. I generally frown upon using locally scoped functions across multiple modules (e.g. APIToken has nothing to do with Monitor), and given that its for a specific field default I think its fine to duplicate the code

src/sentry/tasks/check_monitors.py

dcramer · 2019-01-23T00:50:28Z

This might be functional enough that we could merge in and prototype

src/sentry/api/endpoints/monitor_checkin_details.py

dcramer · 2019-01-23T17:24:41Z

Two schema things we should agree on:

should schedule be a column (i think its ok to keep it uniform in syntax)
should we use 'duration' or simply 'date_finished' for checkin's

src/sentry/models/monitorcheckin.py

src/sentry/models/monitorlocation.py

src/sentry/db/models/fields/guid.py

dcramer · 2019-01-23T21:25:53Z

So one more decision: do we just use guid as pk?

dcramer · 2019-01-23T22:22:13Z

After convo in Slack - let's keep guid and id as there are pros/cons of replacing id with guid that we'd prefer to not deal w/ today.

mattrobenolt · 2019-01-23T22:58:50Z

src/sentry/tasks/check_monitors.py

Definitely need an index for this query. Probably fully on (type, status, next_checkin). Can get away without having status in there if we think that case is pretty rare.

I think we can ignore status and just NULL the field if you delete/disable the monitor.

markstory · 2019-01-23T23:18:38Z

should schedule be a column (i think its ok to keep it uniform in syntax)

I would prefer to have as many real columns as we can. Having data in json blobs makes future schema changes and record updates harder as we have to decode the json blob.

should we use 'duration' or simply 'date_finished' for checkin's

I think storing both would be ideal. Having duration allows the creation of alert rules based on the job runtime without having to infer based on the schedule + completion time. Duration is also useful for being able to know when we have missed a check-in with more confidence.

dcramer · 2019-01-23T23:37:03Z

@markstory the json decoding isn’t an issue but we aren’t certain on how scheduling should be defined for other monitors. For example it’s much more complex to parse ceontqb schedules that it is “run_every_minutes=1. I also wouldn’t want columns if they weren’t uniform types and used by every subschema.

Not sure I understand your point on the duration bit. I do think we might need a date_updated to understand the last communication on an individual check in.

dcramer · 2019-01-24T00:46:58Z

If anyone has anything else they think is hard to take back before prototyping this with our own stuff let me know and we can resolve before merge, otherwise my goal is to get this into prod and wire up some of our jobs // start talking to some customers more about what this might look like down the road.

dcramer · 2019-01-24T00:49:22Z

src/sentry/models/monitor.py

also note: @alex-hofsteede @bretthoerner @tkaemming this is what i was talking about where its really hard to internally generate a real event and insert it into the pipeline. In this case I wanted to bypass rate limits and standard tracking since we wouldn't want to enforce them with this. I realize we could refactor it, but I also didn't want to be responsible for that. Something we should consider going forward within our internal APIs.

dcramer · 2019-01-24T18:07:26Z

I added date_updated to checkins which will let us manage a long-running task more effectively. It could enable the kind of reporting that says "this is still running" vs "it just hasn't checked in as finished".

dcramer · 2019-01-24T18:12:57Z

Also added the 'monitor' context type so it will automatically register the tag.

dcramer · 2019-01-24T18:25:27Z

Need to fix UUIDField for sqlite yet

- Add support for schedule-based push monitors - And endpoint for creating and updating checkins (DSN-based auth supported) - Generate a simplistic message event upon monitor failure

dcramer · 2019-01-24T18:48:26Z

Updated UUIDField to work in sqlite - a few changes from pgfields:

not unique by default (doesnt make sense to me why we'd do that, since a UUID might be an fkey to another UUID)
swaps out with CHAR(32) in non-postgres dbs, and stores as UUID.hex
register uuid adapter at runtime, once

dcramer · 2019-01-24T18:53:39Z

@mitsuhiko can you confirm no issues w/ the way im doing this context type in here? thats the only thing we cant easily take away afaik

mitsuhiko · 2019-01-24T18:57:55Z

@dcramer yap, this is fine. It will also roundtrip unprocessed through the new normalizer untouched but if we want to keep it we will want to add it to the protocol in semaphore so it's validated correctly. You don't have to do that though, but we should not forget to do that (reference: https://github.com/getsentry/semaphore/blob/96ebf37c3fd2a205fcf60569514f8b68aab277c2/general/src/protocol/contexts.rs#L138-L169)

@untitaker cc for the above context addition

dcramer · 2019-01-24T18:59:14Z

maybe ill take a stab at adding it to the rust binding since i havent touched rust before

markstory · 2019-01-24T22:06:31Z

The current suite of datetime/duration fields you have cover all the useful timing bits I think we'll need 👍

dcramer · 2019-01-25T19:30:47Z

chatted in Slack - gonna merge and then take next steps to wire this up to our celery scheduled tasks

dcramer added the WIP label Jan 18, 2019

dcramer force-pushed the feat/monitors branch 4 times, most recently from a5539bb to 4cb4d59 Compare January 18, 2019 21:57

markstory reviewed Jan 21, 2019

View reviewed changes

dcramer force-pushed the feat/monitors branch 5 times, most recently from 9097e9f to 3f6d674 Compare January 23, 2019 00:50

dcramer commented Jan 23, 2019

View reviewed changes

src/sentry/api/endpoints/monitor_checkin_details.py Outdated Show resolved Hide resolved

dcramer force-pushed the feat/monitors branch from 3f6d674 to f8d4354 Compare January 23, 2019 00:59

dcramer removed the WIP label Jan 23, 2019

dcramer force-pushed the feat/monitors branch 2 times, most recently from 317582e to 826bd4d Compare January 23, 2019 17:23

dcramer force-pushed the feat/monitors branch 2 times, most recently from 3d01b38 to f271a9b Compare January 23, 2019 18:56

mitsuhiko reviewed Jan 23, 2019

View reviewed changes

src/sentry/models/monitorcheckin.py Outdated Show resolved Hide resolved

src/sentry/models/monitorlocation.py Outdated Show resolved Hide resolved

src/sentry/db/models/fields/guid.py Outdated Show resolved Hide resolved

dcramer force-pushed the feat/monitors branch from f271a9b to 36caea9 Compare January 23, 2019 21:25

dcramer force-pushed the feat/monitors branch from 36caea9 to 50ff25f Compare January 23, 2019 22:18

dcramer force-pushed the feat/monitors branch 2 times, most recently from f1745f8 to b84402c Compare January 23, 2019 22:47

mattrobenolt reviewed Jan 23, 2019

View reviewed changes

dcramer force-pushed the feat/monitors branch from b84402c to f9e4b8e Compare January 23, 2019 23:02

dcramer commented Jan 24, 2019

View reviewed changes

dcramer force-pushed the feat/monitors branch from f9e4b8e to c90d220 Compare January 24, 2019 18:06

dcramer force-pushed the feat/monitors branch from c90d220 to 7864186 Compare January 24, 2019 18:09

dcramer force-pushed the feat/monitors branch from 7864186 to a9c05ce Compare January 24, 2019 18:29

git showfeat: Initial monitors implementation

2debd39

- Add support for schedule-based push monitors - And endpoint for creating and updating checkins (DSN-based auth supported) - Generate a simplistic message event upon monitor failure

dcramer force-pushed the feat/monitors branch from a9c05ce to 2debd39 Compare January 24, 2019 18:43

dcramer merged commit d27ecf0 into master Jan 25, 2019

dcramer deleted the feat/monitors branch January 25, 2019 19:32

github-actions bot locked and limited conversation to collaborators Dec 20, 2020

Uh oh!

feat: Initial monitors implementation #11602

feat: Initial monitors implementation #11602

Uh oh!

Conversation

dcramer commented Jan 18, 2019

Uh oh!

dcramer commented Jan 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dcramer commented Jan 23, 2019

Uh oh!

Uh oh!

dcramer commented Jan 23, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dcramer commented Jan 23, 2019

Uh oh!

dcramer commented Jan 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markstory commented Jan 23, 2019

Uh oh!

dcramer commented Jan 23, 2019

Uh oh!

dcramer commented Jan 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcramer commented Jan 24, 2019

Uh oh!

dcramer commented Jan 24, 2019

Uh oh!

dcramer commented Jan 24, 2019

Uh oh!

dcramer commented Jan 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dcramer commented Jan 24, 2019

Uh oh!

mitsuhiko commented Jan 24, 2019

Uh oh!

dcramer commented Jan 24, 2019

Uh oh!

markstory commented Jan 24, 2019

Uh oh!

dcramer commented Jan 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dcramer commented Jan 24, 2019 •

edited

Loading