getsentry / sentry-dotnet

Sentry SDK for .NET
https://docs.sentry.io/platforms/dotnet
MIT License
566 stars 203 forks source link

feat: Check-In Upserts #3330

Open bitsandfoxes opened 2 weeks ago

bitsandfoxes commented 2 weeks ago

Using this would look like this


var id = SentrySdk.CaptureCheckIn("my_monitor", CheckInStatus.InProgress, configureMonitorOptions: options =>
{
    options.Interval("0 * * * *");
    // or
    options.Interval(1, MeasurementUnit.Duration.Day);

    options.FailureIssueThreshold = 3;
    options.MaxRuntime = TimeSpan.FromMinutes(5);
    options.Timezone = TimeZoneInfo.FindSystemTimeZoneById("Pacific Standard Time"); // figure out how to deal with OS shenanigans
    options.Owner = "me@there.com";
});
github-actions[bot] commented 2 weeks ago
Fails
:no_entry_sign: Please consider adding a changelog entry for the next release.
Messages
:book: Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Check-In Upserts ([#3330](https://github.com/getsentry/sentry-dotnet/pull/3330))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by :no_entry_sign: dangerJS against 77cdd2fec9fd995d8a6166970b3934982e65c8d4

bruno-garcia commented 2 weeks ago
var monitorConfig = SentryMonitorConfig.CreateCronMonitorConfig("0 * * * *");
monitorConfig.FailureIssueThreshold = 3;
monitorConfig.MaxRuntime = TimeSpan.FromMinutes(5);
monitorConfig.Timezone = TimeZoneInfo.FindSystemTimeZoneById("Pacific Standard Time");
monitorConfig.Owner = "me@there.com";

This reads like an old C# API and seems very ackward to me. Are all those properties optional? Threshold, MaxRuntime, TimeZone? (also casing in C# for this is TimeZone as its two words, as seen on TimeZoneInfo above too. Please validate casing before locking in a new PR (as in, merging a PR))

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. Take what's required, on the first set of args. And follow by optional parameters. Additionally you can consider an overloap that takes a full object.

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 SentrySdk.Init(o => o.Crons(interval, threshold, etc) ?