feat(monitors): Ingestion for basic check-in payloads #1886
Conversation
relay-crons/src/lib.rs
Outdated
| let mut checkin = serde_json::from_slice::<Checkin>(payload)?; | ||
|
|
||
| // Missed status cannot be ingested, this is computed on the server. | ||
| if checkin.status == CheckinStatus::Missed { |
There was a problem hiding this comment.
I wonder if there will be cases we want to allow the user to send missed checkins.
This is fine for now
relay-crons/src/lib.rs
Outdated
|
|
||
| /// Identifier of the monitor for this checkin. | ||
| #[serde(serialize_with = "uuid_simple")] | ||
| monitor_id: Uuid, |
There was a problem hiding this comment.
I believe this is going to become monitor_slug as I mentioned to you.
Let me get confirmation for getsentry/sentry#45166 first though.
There was a problem hiding this comment.
Sounds good. I'd propose we merge as-is and follow up with a set of PRs for Relay + Sentry once this has cleared up.
jan-auer
force-pushed
the
feat/crons-mvp
branch
from
758106b to
0fbece7
Compare
relay-crons/src/lib.rs
Outdated
| /// Status of this checkin. Defaults to `"unknown"`. | ||
| status: CheckinStatus, |
There was a problem hiding this comment.
Don't we need a #[serde(default)] here to derive the default in case the field is missing from the payload?
There was a problem hiding this comment.
I've had this discussion with @cleptric . The current endpoint requires this field. We're thinking about making it optional as long as we can still break this API. @evanpurkhiser would you like it to be optional right away?
jan-auer
changed the title
|
I've updated naming to be consistently:
|
* master: doc(py): Add changelog entries (#1900) fix(build): Run check when PR is ready for review (#1899) chore(project_local): Allow to follow symlinks for projects configs (#1891) ref(project): Skip serializing default fields (#1887) chore(build): Run changelog check for draft PRs (#1897) chore(sentry): Add environment config option (#1890) feat(scrubbing): Scrub `span.data.http.query` with default scrubbers (#1889)
| check_in.status = CheckInStatus::Unknown; | ||
| } | ||
|
|
||
| Ok(serde_json::to_vec(&check_in)?) |
There was a problem hiding this comment.
@jan-auer did you still want to make this copy-on-write?
There was a problem hiding this comment.
No, I intentionally removed that because we always need to normalize the status enum.
There was a problem hiding this comment.
Right, because the deserializer maps to Unknown, gotcha. You could even remove the Missed variant then, though it also makes sense to keep that explicit.
| InProgress, | ||
| /// Monitor did not check in on time. | ||
| Missed, | ||
| /// No status was passed. |
There was a problem hiding this comment.
| /// No status was passed. | |
| /// No status was passed. | |
| /// | |
| /// When we enable check-in deserialization in external relays, this should become `Unknown(String)` for forward compatibility. |
There was a problem hiding this comment.
Not necessarily, that depends on the kind of normalization we're doing in external Relays. It may be safer to keep it as "Unknown" rather than applying the wrong normalization (same to the metrics extraction version).
| true | ||
| } | ||
| Err(error) => { | ||
| // TODO: Track an outcome. |
There was a problem hiding this comment.
Are we OK with not tracking outcomes here?
There was a problem hiding this comment.
Yes, the backend also doesn't track outcomes yet, and it's not yet clear whether we want to track per check-in update or only per check-in.
This adds ingestion for cron monitor checkin payloads through Relay. Initially, this just covers creating and updating checkins, but not monitors. Rate limits can be set and enforced, but there will be no outcomes tracked.
processingmodeingest-monitors)Follow-up:
See getsentry/sentry#45079 for the consumer.
Requires https://github.com/getsentry/ops/pull/6226