Skip to content
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

Merged
merged 37 commits into from
May 29, 2024
Merged

feat: Check-In Upserts #3330

merged 37 commits into from
May 29, 2024

Conversation

bitsandfoxes
Copy link
Contributor

@bitsandfoxes bitsandfoxes commented Apr 25, 2024

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 = "America/Los_Angeles";
    options.Owner = "[email protected]";
});

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.

Copy link
Contributor

github-actions bot commented Apr 25, 2024

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 8875318

Base automatically changed from fix/crons-1 to main April 29, 2024 14:12
@bruno-garcia
Copy link
Member

var monitorConfig = SentryMonitorConfig.CreateCronMonitorConfig("0 * * * *");
monitorConfig.FailureIssueThreshold = 3;
monitorConfig.MaxRuntime = TimeSpan.FromMinutes(5);
monitorConfig.Timezone = TimeZoneInfo.FindSystemTimeZoneById("Pacific Standard Time");
monitorConfig.Owner = "[email protected]";

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) ?

@bitsandfoxes bitsandfoxes marked this pull request as ready for review May 22, 2024 07:47
Copy link
Collaborator

@jamescrosswell jamescrosswell left a 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.

src/Sentry/Extensibility/HubAdapter.cs Outdated Show resolved Hide resolved
src/Sentry/SentryMonitorOptions.cs Outdated Show resolved Hide resolved
src/Sentry/SentryMonitorOptions.cs Show resolved Hide resolved
Copy link
Collaborator

@jamescrosswell jamescrosswell left a comment

Choose a reason for hiding this comment

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

Looking very near ready. See my comment/question about whether we need the two overloads and/or I'd recommend we use a source generated regex in net8.0 if we do retain both overloads.

Comment on lines 66 to 68
private readonly Regex _crontabValidation = new(
@"^(\*|([0-5]?\d))(\s+)(\*|([01]?\d|2[0-3]))(\s+)(\*|([1-9]|[12]\d|3[01]))(\s+)(\*|([1-9]|1[0-2]))(\s+)(\*|([0-7]))$",
RegexOptions.Compiled | RegexOptions.CultureInvariant);
Copy link
Collaborator

Choose a reason for hiding this comment

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

On .NET 8 we could use Source Generated regular expressions for better performance.

We do this for the BuiltInSystemDiagnosticsMeters, for example.

src/Sentry/SentryMonitorOptions.cs Show resolved Hide resolved
Copy link
Collaborator

@jamescrosswell jamescrosswell left a comment

Choose a reason for hiding this comment

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

LGTM. Just the source generators for regex, if you feel like making it as performant as possible.

@bitsandfoxes bitsandfoxes merged commit 37eb4a5 into main May 29, 2024
20 checks passed
@bitsandfoxes bitsandfoxes deleted the fix/crons-2 branch May 29, 2024 13:56
/// <summary>
/// A tz database string representing the timezone which the monitor's execution schedule is in (i.e. "America/Los_Angeles").
/// </summary>
public string? Timezone { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

IIRC I raised this before but it's TimeZone: not Timezone: https://learn.microsoft.com/en-us/dotnet/api/system.timezone?view=net-8.0

2 words: time zone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants