Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

msp: add infra and runtime support for job checkins#62508

Merged
bobheadxi merged 7 commits into
mainfrom
msp-job-checkin
May 9, 2024
Merged

msp: add infra and runtime support for job checkins#62508
bobheadxi merged 7 commits into
mainfrom
msp-job-checkin

Conversation

@bobheadxi

@bobheadxi bobheadxi commented May 7, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Closes CORE-21 - allows jobs to register check-ins using Sentry when they are configured as cron jobs: https://docs.sentry.io/product/crons/, for a nice view of "is my job running or nah" without using GCP's less-than-beautiful console views

  1. Adds the configured schedule and deadline as environment variables for MSP jobs
  2. Adds a contract mechanism for checking in, for example:
	func work(ctx context.Context) (err error) {
		done, err := c.Diagnostics.JobExecutionCheckIn(ctx)
		if err != nil { /* failed to register check-in */ }
		defer done(err)

		// ... do work
	}

Test plan

TestJobExecutionCheckIn_SENTRY_DSN='...' go test -v ./runtime/contract

image

In Slack:

image

It appears the message is not currently customizable: https://develop.sentry.dev/sdk/check-ins/

@cla-bot cla-bot Bot added the cla-signed label May 7, 2024
@bobheadxi bobheadxi force-pushed the msp-job-checkin branch 2 times, most recently from ca3dbd3 to 7166875 Compare May 7, 2024 21:49
@bobheadxi bobheadxi marked this pull request as ready for review May 7, 2024 21:51
@bobheadxi bobheadxi requested review from a team, chrsmith, jac and rafax May 7, 2024 21:51
rafax
rafax approved these changes May 8, 2024
View reviewed changes

@rafax rafax left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable, but I'm not the expert on msp

Comment thread lib/managedservicesplatform/runtime/contract/diagnostics.go
OpenTelemetry opentelemetry.Config
sentryDSN *string

cronSchedule *string

@rafax rafax May 8, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is schedule really optional? Is there a default?

@bobheadxi bobheadxi May 8, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's optional in the sense that only jobs, and only jobs that have a schedule configured at all (provisioning job + other MSP resources, and then DIY-ing the trigger is an option)

@rafax rafax May 8, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok fair, didn't notice this is shared between services and jobs

@bobheadxi bobheadxi May 8, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added docstrings to clarify :)

@unknwon unknwon May 9, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

----- Split of topic -----

It's kinda surpising to see cron schedule is part of "diagnostics"? 🤔 or is it for exposing the cron schedule for diagnostics?

@bobheadxi bobheadxi May 9, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's only exposing the cron schedule for diagnostics, we don't (can't) actually apply the cron schedule from the runtime

unknwon
unknwon approved these changes May 9, 2024
View reviewed changes

@unknwon unknwon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL!

Comment thread lib/managedservicesplatform/runtime/contract/diagnostics.go Outdated
Comment thread lib/managedservicesplatform/runtime/contract/diagnostics.go
OpenTelemetry opentelemetry.Config
sentryDSN *string

cronSchedule *string

@unknwon unknwon May 9, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

----- Split of topic -----

It's kinda surpising to see cron schedule is part of "diagnostics"? 🤔 or is it for exposing the cron schedule for diagnostics?

Comment thread lib/managedservicesplatform/runtime/contract/diagnostics.go Outdated
Comment thread lib/managedservicesplatform/runtime/contract/diagnostics.go Outdated
bobheadxi and others added 2 commits May 9, 2024 09:58
@bobheadxi bobheadxi merged commit 4d64559 into main May 9, 2024
@bobheadxi bobheadxi deleted the msp-job-checkin branch May 9, 2024 17:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@chrsmith chrsmith Awaiting requested review from chrsmith

@jac jac Awaiting requested review from jac

2 more reviewers

@rafax rafax rafax approved these changes

@unknwon unknwon unknwon approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bobheadxi @rafax @unknwon