New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Check-In Upserts #3330
base: main
Are you sure you want to change the base?
feat: Check-In Upserts #3330
Conversation
|
This reads like an old C# API and seems very ackward to me. Why do we need an email for owner? I imagine that's optional? Since we use the DSN the cron job gets tied to a project which has a team/owner already. Consider what's required value, and what's optional. But finally, my understanding of upset was that it would take the same API we use to make a normal check-in. Can we figure out interval through that? The key feature/ask here is that I don't need to create something upfront. It'll take a new check-in it'll create a monitor for it. Maybe it requires a few check-ins to figure out the interval this job is running at before it can start alerting if check-ins are failing. cc @gaprl does it do that by any chance? If we need some full blown API to create the cron job, do we run that once each time the app starts? If so, should that be on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we discussed, I think we need to add some more argument validation to ensure users can only configure crontab values that Sentry will accept but otherwise looking good.
{ | ||
throw new ArgumentException("You tried to set the interval twice. The Check-Ins interval is supposed to be set only once."); | ||
} | ||
|
||
_type = SentryMonitorScheduleType.Interval; | ||
_interval = interval; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the cronTab
argument to the overload above, we should either check that the interval + unit here are consistent with the accepted values in the Sentry protocol. Again, although we need to serialize this as a string in order to communicate it to Sentry, internally we could have a more structured representation of this, if that made it easier for the user to express and for our SDK to validate.
Using this would look like this
I wish I could use
TimeZoneInfo
but Windows has its own thing going on and supports IANA Timezone IDs out of the box starting with .NET 6 (source)Checking with the other SDKs I don't really see the reason to build our own timezone converter for .NET 5 and older if the user has to specify via
string
which timezone anyway.