feat: Added cron specific endpoint #2153

mitsuhiko · 2023-05-24T11:05:09Z

Implements a cron specific API endpoint. The check_in_id is changed to nullable in this. If a check_in_id is not supplied, the consumer will get the nil check_in_id which is an alias for "latest". I decided not to tackle the base62 implementation, mostly because now that this reuses the generic infrastructure it might be quite confusing if we end up with different key formats.

The URL format is also slightly different than in the original ticket. The following options are supported:

curl -u sentry_key: https://oXXX.ingest.sentry.io/api/cron/{monitor_slug}/?status={status}
curl https://oXXX.ingest.sentry.io/api/cron/{monitor_slug}/?status={status}&sentry_key={sentry_key}
curl https://oXXX.ingest.sentry.io/api/cron/{monitor_slug}/{sentry_key}/?status={status}

Work remaining:

tests
latest for check_in_id
requires split up of some monitors code for non processing builds

Fixes #2101
Fixes getsentry/sentry#49203

relay-server/src/extractors/request_meta.rs

evanpurkhiser · 2023-05-30T20:44:57Z

Looking over this I think we're good to merge this.

I'll create some follow up tickets since not long after we'll likely also want to support POSTing to this endpoint to support all the features we have in the existing endpoints (passing monitor configurations for upserts)

mitsuhiko · 2023-05-30T22:08:55Z

@evanpurkhiser fwiw I did implement some parameters as query strings. Eg as implemented you can pass ?check_in_id=.... for instance. (See all in CronQuery). But I do agree that a post endpoint would be nice.

olksdr

Requesting changes, since I have still few questions I would like to clarify before merging this into the Relay.

olksdr · 2023-05-31T05:22:09Z

CHANGELOG.md

@@ -6,6 +6,8 @@

 - Use different error message for empty strings in schema processing. ([#2151](https://github.com/getsentry/relay/pull/2151))

+- Relay now supports a simplified cron check-in API.  ([#2153](https://github.com/getsentry/relay/pull/2153))


There is the unnecessary new line before this record

olksdr · 2023-05-31T05:25:11Z

relay-server/src/endpoints/mod.rs

+        .route("/api/cron/:monitor_slug/:sentry_key", cron::route(config))
+        .route("/api/cron/:monitor_slug/:sentry_key/", cron::route(config))
+        .route("/api/cron/:monitor_slug", cron::route(config))
+        .route("/api/cron/:monitor_slug/", cron::route(config))


Why do we have to support trailing / and no trailing / URLs? Have the routes without trailing slashes been already exposed to customers and customer got used to using them by now?

If not, IMO we have to keep our endpoints consistent, and steer the customers to use only one defined route. And currently we have / at the end, which I would also lean to.

I generally think, from a devex perspective, it's confusing to get errors for missing (or extra) slashes at the end of an URL. Although, I would have imagined we handle this in a cleaner way that adding both URL variants.

Agree would be nice to handle this in a more general way

olksdr · 2023-05-31T05:29:50Z

relay-server/src/extractors/request_meta.rs

@@ -479,6 +480,30 @@ fn auth_from_parts(req: &Parts, path_key: Option<String>) -> Result<Auth, BadEve
        auth = Some(header.parse::<Auth>()?);
    }

+    // try to get authentication info from basic auth
+    if let Some(basic_auth) = req


What the reason even to add basic auth?

Using basic auth is super confusion to me to be honest.

Especially when you look at the format users should use to make it work. And I talk about the part when you have yo prove the key (curl -u sentry_key: ) this colon after sentry_key is super easy to miss and mess up your curl command. IMO it makes this very error-prone to the customer.

Much easier and more explicit way would be to use the header which we support or path param, which is also supported.

We're adding this as a bit of an experiment for crons to feel out the ergonomics and DX doing it this way.

I don't think we're planning to immediately tell customers to use this. I agree with the points you make though @olksdr, I think it's not unlikely this will get removed.

Please, add the note here, which explains why this added and that this is just an experiment and will be removed in the follow-up.

@olksdr fwiw I think basic auth should be generally supported, beyond this experiment, entirely independent of this. From the original x-sentry-auth header basically nothing remains and we already support auth purely by ?sentry_key in the URL. At this point I would honestly love to see us supporting just Bearer and basic auth with just the public key.

cc @jan-auer

olksdr · 2023-05-31T05:30:22Z

relay-server/src/endpoints/cron.rs

+        Ok(_) | Err(BadStoreRequest::RateLimited(_)) => (),
+        Err(error) => return Err(error),
+    };
+    // What do we want to return?


Do we want to have this comment here?

@evanpurkhiser can we remove this comment? And return what you want to return? Or add a note here to followup and why the return value cannot be decided right now.

evanpurkhiser · 2023-06-02T18:09:41Z

@olksdr Anything else you wanted to follow up with on this? @mitsuhiko I think we're good to go ahead and merge this in and start playing with it yeah?

This is to support getsentry/relay#2153 Provdes a means to creating check-ins without giving a check_in_id in the consumer payload. Documentation for this is here getsentry/develop#950 Fixes: #49664

olksdr

Left few comments.
Will approve to unblock you, if the open questions will be followed up on.

And also maybe @jan-auer wants also have a quick review

olksdr · 2023-06-03T04:27:08Z

relay-server/src/extractors/request_meta.rs

@@ -479,6 +480,30 @@ fn auth_from_parts(req: &Parts, path_key: Option<String>) -> Result<Auth, BadEve
        auth = Some(header.parse::<Auth>()?);
    }

+    // try to get authentication info from basic auth
+    if let Some(basic_auth) = req


Please, add the note here, which explains why this added and that this is just an experiment and will be removed in the follow-up.

olksdr · 2023-06-03T04:29:38Z

relay-server/src/endpoints/cron.rs

+        Ok(_) | Err(BadStoreRequest::RateLimited(_)) => (),
+        Err(error) => return Err(error),
+    };
+    // What do we want to return?


@evanpurkhiser can we remove this comment? And return what you want to return? Or add a note here to followup and why the return value cannot be decided right now.

olksdr

Left few comments.
Will approve to unblock you, if the open questions will be followed up on.

And also maybe @jan-auer wants also have a quick review

evanpurkhiser · 2023-06-05T18:02:27Z

tests/integration/test_crons.py

+    monitor_slug = "my-monitor"
+    public_key = relay.get_dsn_public_key(project_id)
+    basic_auth = base64.b64encode((public_key + ":").encode("utf-8")).decode("utf-8")
+    relay.post(


Just noticed, this should be get right @mitsuhiko ?

The endpoint supports either.

Ah cool. I'll expand on POST in a future PR.

evanpurkhiser · 2023-06-06T17:10:36Z

Will follow up on your comments @olksdr Thanks!

* master: (32 commits) feat(txname): Mark all URL transactions as sanitized (#2210) fix(profiles): Enforce profile quotas on metrics buckets (#2212) revert(spans): Move span metrics back to transactions namespace (#2206) instr(metrics): Add values to invalid metric errors (#2201) ref(spans): Move span metrics to spans namespace (#2205) chore(protocol): Add missing py/changelog entry for lock property (#2203) build(py): Move pytest.ini to tests directory (#2204) ref: Use serde collect_str to avoid allocations (#2149) chore(gocd): Auto approve deploy relay-pop on check success (#2200) test(document-pii): Fix flaky test (#2198) feat(spans): Extract transaction `user` into span's databag (#2197) feat(spans): Extract `release` tag from the transaction to the span (#2189) ref(metrics-extraction): Split span and transaction metric extraction (#2191) ref(crons): Support GET in the check-in endpoint (#2192) ref(spans): Introduce opinionated casing in span metrics (#2193) chore(py): Bump `requests` lib version (#2194) doc: document pii fields (#1972) feat: Added cron specific endpoint (#2153) feat(profiling): Extract app identifier from app context (#2172) feat(ci): Add CI-check for pop-deploys (#2184) ...

feat: Added cron specific endpoint

cbd1e4d

mitsuhiko requested a review from a team May 24, 2023 11:05

mitsuhiko marked this pull request as draft May 24, 2023 11:05

mitsuhiko added 2 commits May 24, 2023 13:05

ref: remove unused var

c78de46

fix: always compile in monitors

4261abe

iker-barriocanal assigned mitsuhiko May 24, 2023

mitsuhiko added 3 commits May 24, 2023 15:51

test: Added basic monitors test

3cb68e7

test: Added failing test for URL embedded auth

1ea9281

fix: path handling for crons

ec22c0f

mitsuhiko marked this pull request as ready for review May 24, 2023 15:06

mitsuhiko mentioned this pull request May 24, 2023

[Crons] Implement a monitor checkin API endpoint #2101

Closed

mitsuhiko added 2 commits May 24, 2023 17:08

doc: Added changelog entry

867f076

Merge branch 'master' into feature/cron-endpoint

8ba655c

evanpurkhiser reviewed May 30, 2023

View reviewed changes

relay-server/src/extractors/request_meta.rs Show resolved Hide resolved

evanpurkhiser mentioned this pull request May 30, 2023

Crons: Support latest (empty) check_in_id in the monitor consumer getsentry/sentry#49664

Closed

olksdr requested changes May 31, 2023

View reviewed changes

evanpurkhiser mentioned this pull request Jun 1, 2023

feat(crons): Support empty check_in_id in consumer getsentry/sentry#50198

Merged

olksdr approved these changes Jun 3, 2023

View reviewed changes

evanpurkhiser reviewed Jun 5, 2023

View reviewed changes

evanpurkhiser merged commit 9c7f7e8 into master Jun 6, 2023

evanpurkhiser deleted the feature/cron-endpoint branch June 6, 2023 17:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Added cron specific endpoint #2153

feat: Added cron specific endpoint #2153

mitsuhiko commented May 24, 2023 •

edited by evanpurkhiser

Loading

evanpurkhiser commented May 30, 2023

mitsuhiko commented May 30, 2023

olksdr left a comment

olksdr May 31, 2023

olksdr May 31, 2023

jernejstrasner May 31, 2023

evanpurkhiser Jun 1, 2023

olksdr May 31, 2023

evanpurkhiser Jun 1, 2023

olksdr Jun 3, 2023

mitsuhiko Jun 6, 2023

olksdr May 31, 2023

olksdr Jun 3, 2023

evanpurkhiser commented Jun 2, 2023

olksdr left a comment

olksdr Jun 3, 2023

olksdr Jun 3, 2023

olksdr left a comment

evanpurkhiser Jun 5, 2023

mitsuhiko Jun 6, 2023

evanpurkhiser Jun 6, 2023

evanpurkhiser commented Jun 6, 2023

		@@ -6,6 +6,8 @@

		- Use different error message for empty strings in schema processing. ([#2151](https://github.com/getsentry/relay/pull/2151))

		- Relay now supports a simplified cron check-in API. ([#2153](https://github.com/getsentry/relay/pull/2153))

feat: Added cron specific endpoint #2153

feat: Added cron specific endpoint #2153

Conversation

mitsuhiko commented May 24, 2023 • edited by evanpurkhiser Loading

evanpurkhiser commented May 30, 2023

mitsuhiko commented May 30, 2023

olksdr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanpurkhiser commented Jun 2, 2023

olksdr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olksdr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanpurkhiser commented Jun 6, 2023

mitsuhiko commented May 24, 2023 •

edited by evanpurkhiser

Loading