feat(crons): initial cron support #661

aldy505 · 2023-07-04T15:13:00Z

Closes #659

This implementations looks up to:

aldy505 · 2023-07-04T15:17:44Z

client.go

+// CaptureCheckIn captures a check in.
+func (client *Client) CaptureCheckIn(checkIn *CheckIn, monitorConfig *MonitorConfig, scope EventModifier) *EventID {


There was a discussion in Discord (#off-topic) between me and @cleptric about what function name to implement (as the PHP and Node implementation is quite different), where Michi suggested to follow the PHP implementation as the event is the check in already. As for Go SDK, if this implementation folows PHP's, this would be something like:

event := &sentry.Event{ // fill payloads } event.SetCheckIn(checkInPayload) eventId := sentry.CaptureEvent(event)

It's kind of awkward and confusing for me, so I went ahead with the CaptureCheckIn as a method of Client struct.

aldy505 · 2023-07-04T15:19:29Z

interfaces.go

+	// The fields below are only relevant for crons/check ins
+
+	CheckInID     string         `json:"check_in_id,omitempty"`
+	CheckIn       *CheckIn       `json:"check_in,omitempty"`
+	MonitorConfig *MonitorConfig `json:"monitor_config,omitempty"`


If I follow how transactions are keeping their data, it would mean that every entry on CheckIn would be spread out and might mess with other fields in the future, so I keep it enclosed here with omitempty tag.

aldy505 · 2023-07-04T15:20:19Z

interfaces.go

+	if e.MonitorConfig != nil {
+		checkIn.MonitorConfig = &MonitorConfig{}
+		checkIn.MonitorConfig.Schedule = e.MonitorConfig.Schedule
+		checkIn.MonitorConfig.CheckInMargin = e.MonitorConfig.CheckInMargin
+		checkIn.MonitorConfig.MaxRuntime = e.MonitorConfig.MaxRuntime
+		checkIn.MonitorConfig.Timezone = e.MonitorConfig.Timezone
+	}


Yes I know it looks bad. It's 10 PM on Tuesday and I can't think of anything better right now. So sorry 🙈

tonyo · 2023-07-05T17:59:43Z

Thanks @aldy505 , we'll have a look this/next week 👍

codecov · 2023-07-10T11:06:24Z

Codecov Report

Patch coverage: 86.48% and project coverage change: +0.20 🎉

Comparison is base (b6dfea7) 80.49% compared to head (5c43228) 80.70%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #661      +/-   ##
==========================================
+ Coverage   80.49%   80.70%   +0.20%     
==========================================
  Files          42       44       +2     
  Lines        4333     4416      +83     
==========================================
+ Hits         3488     3564      +76     
- Misses        717      724       +7     
  Partials      128      128

Impacted Files	Coverage Δ
sentry.go	`29.31% <0.00%> (-1.60%)`	⬇️
check_in.go	`73.33% <73.33%> (ø)`
hub.go	`86.50% <75.00%> (-0.74%)`	⬇️
client.go	`87.92% <100.00%> (+0.88%)`	⬆️
interfaces.go	`93.30% <100.00%> (+0.72%)`	⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

tonyo

Overall looking good 👍
Left some comments, but nothing super major.

sentry.go

hub.go

tonyo · 2023-07-10T10:27:29Z

client.go

+// CaptureCheckIn captures a check in.
+func (client *Client) CaptureCheckIn(checkIn *CheckIn, monitorConfig *MonitorConfig, scope EventModifier) *EventID {
+	event := client.EventFromCheckIn(checkIn, monitorConfig)
+	return client.processEvent(event, nil, scope)


Let's maybe make it consistent with other Client.Capture* methods.

Suggested change

return client.processEvent(event, nil, scope)

return client.CaptureEvent(event, nil, scope)

I'll have to reread the code before committing the suggested changes. I don't remember if it's okay to call that method from here.

check_in.go

client_test.go

interfaces.go

check_in.go

Co-authored-by: Anton Ovchinnikov <anton@tonyo.info>

aldy505 · 2023-07-17T10:43:24Z

I'll address any errors on CI in a few hours.

tonyo · 2023-07-17T11:22:08Z

Failures on Windows can be ignored for now, seems like it's only a test that has been flaky recently.

tonyo · 2023-07-17T13:31:22Z

check_in.go

+	// The status of the check-in.
+	Status CheckInStatus `json:"status"`
+	// The duration of the check-in. Will only take effect if the status is ok or error.
+	Duration time.Duration `json:"duration,omitempty"`


What I realized is that we don't have an option to pass an existing CheckInID via CheckIn now.
At the moment we basically create a new CheckInID every time, so there's no way to link the "in-progress" and "ok"/"error" status.
Could you handle this case please?

Done. I might want to add a test case for this one, or an example.

tonyo · 2023-07-17T13:38:04Z

Another thing that we'll have to do eventually is to update the docs, basically adding Go to this list: https://docs.sentry.io/product/crons/getting-started/
@aldy505 let us know if you're ready to contribute to the docs too; that'd speed up the release/adoption of the feature.

tonyo · 2023-07-18T15:51:10Z

Thanks a lot @aldy505 👍 🎉

feat(crons): initial cron support

81e5401

aldy505 commented Jul 4, 2023

View reviewed changes

cleptric added Status: In Progress Type: Feature labels Jul 4, 2023

cleptric requested review from cleptric and tonyo July 4, 2023 15:31

aldy505 and others added 2 commits July 9, 2023 12:25

test: set CheckInID to static const

e0e2ccf

Merge branch 'master' into feat/crons/capture-check-in

b75b2bb

tonyo self-assigned this Jul 10, 2023

tonyo suggested changes Jul 10, 2023

View reviewed changes

aldy505 and others added 5 commits July 10, 2023 19:12

Update sentry.go

666c5e7

Co-authored-by: Anton Ovchinnikov <anton@tonyo.info>

Update hub.go

c68637b

Co-authored-by: Anton Ovchinnikov <anton@tonyo.info>

Update check_in.go

9172741

Co-authored-by: Anton Ovchinnikov <anton@tonyo.info>

Update check_in.go

3c9fc2d

Co-authored-by: Anton Ovchinnikov <anton@tonyo.info>

feat: modify duration to time.Duration; add test for CrontabSchedule

cb77d96

aldy505 requested a review from tonyo July 16, 2023 01:18

Merge branch 'master' into feat/crons/capture-check-in

20974d2

aldy505 added 2 commits July 17, 2023 19:19

chore: resolve lint errors

5ec9e0b

test: check in on hub

73d3bdd

tonyo reviewed Jul 17, 2023

View reviewed changes

aldy505 added 3 commits July 17, 2023 21:05

ref(cron): take check in id as an optional user input

f674960

fix: EventID should be equal to CheckInID

3c0381e

ref(cron): enable check in from previous CheckInID

5c43228

tonyo approved these changes Jul 18, 2023

View reviewed changes

tonyo merged commit 4b3a135 into getsentry:master Jul 18, 2023

This was referenced Jul 21, 2023

Documentation for Crons #671

Closed

fix(crons): Pass correct event types, fix checkin_margin field, add example #674

Merged

aldy505 deleted the feat/crons/capture-check-in branch September 3, 2023 02:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(crons): initial cron support #661

feat(crons): initial cron support #661

aldy505 commented Jul 4, 2023

aldy505 Jul 4, 2023

aldy505 Jul 4, 2023

aldy505 Jul 4, 2023

tonyo commented Jul 5, 2023

codecov bot commented Jul 10, 2023 •

edited

Loading

tonyo left a comment

tonyo Jul 10, 2023

aldy505 Jul 10, 2023

aldy505 commented Jul 17, 2023

tonyo commented Jul 17, 2023

tonyo Jul 17, 2023

aldy505 Jul 17, 2023

tonyo commented Jul 17, 2023

tonyo commented Jul 18, 2023

		// CaptureCheckIn captures a check in.
		func (client Client) CaptureCheckIn(checkIn CheckIn, monitorConfig MonitorConfig, scope EventModifier) EventID {

	return client.processEvent(event, nil, scope)
	return client.CaptureEvent(event, nil, scope)

feat(crons): initial cron support #661

feat(crons): initial cron support #661

Conversation

aldy505 commented Jul 4, 2023

aldy505 Jul 4, 2023

Choose a reason for hiding this comment

aldy505 Jul 4, 2023

Choose a reason for hiding this comment

aldy505 Jul 4, 2023

Choose a reason for hiding this comment

tonyo commented Jul 5, 2023

codecov bot commented Jul 10, 2023 • edited Loading

Codecov Report

tonyo left a comment

Choose a reason for hiding this comment

tonyo Jul 10, 2023

Choose a reason for hiding this comment

aldy505 Jul 10, 2023

Choose a reason for hiding this comment

aldy505 commented Jul 17, 2023

tonyo commented Jul 17, 2023

tonyo Jul 17, 2023

Choose a reason for hiding this comment

aldy505 Jul 17, 2023

Choose a reason for hiding this comment

tonyo commented Jul 17, 2023

tonyo commented Jul 18, 2023

codecov bot commented Jul 10, 2023 •

edited

Loading