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

Add ability to schedule Event Definitions with a cron expression #19563

Conversation

kingzacko1
Copy link
Contributor

@kingzacko1 kingzacko1 commented Jun 5, 2024

Adds support for cron scheduling of Filter & Aggregation event definitions. Its just a plain textbox for the cron expression. Frontend will need to work their magic if we want a GUI-driven cron expression generator in product. In lieu of a full-blown cron expression generator, I've added a /validate/cron_expression API endpoint to allow the frontend an easy way to validate the current expression in the textbox. It validates the expression and if successful returns a description of the cron expression so users can confirm that is how they wish to schedule the event.

NOTE: After testing this PR if you want to keep all the event defintions you were testing with you should change all event definitions back to not using cron scheduling and then disable them. This should cleanup any cron related fields in the DB when switching back to other branches without these changes. If you don't care to keep the event definitions, just delete them.

Description

  • Add a cron_expression and cron_timezone field to AggregationEventProcessorConfig to support cron scheduling
    image
  • Fixes CronJobSchedule.calculateNextTime to truly calculate next time instead of next future time
    • Reporting seems to rely on this method returning the next future time. Will need to refactor to actually use a method that returns that.
  • Moves logic describing event definition scheduling to the backend to take advantage of the CronDescriptor.describe method. It's a little wonky sometimes, but better than trying to handle on the frontend.
    image

Motivation and Context

Graylog2/graylog-plugin-enterprise#6598
#19365
Also possibly:
#12364
Graylog2/support#46

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.

@kingzacko1 kingzacko1 marked this pull request as ready for review June 10, 2024 16:32
Copy link
Member

@bernd bernd left a comment

Choose a reason for hiding this comment

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

A few backend-related notes.

// If the job uses cron scheduling, we need to instead calculate the nextTo field based on the current to
// field as it is possible to skip contiguous time ranges with cron scheduling.
DateTime nextTo = config.isCron() ?
scheduleStrategies.nextTime(ctx.trigger(), to).orElse(to.plus(config.processingHopSize())) :
Copy link
Member

Choose a reason for hiding this comment

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

When does #nextTime(trigger, to) return an empty Optional? The method's Javadoc says that the trigger shouldn't fire anymore if it returns an empty Optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this context, it would theoretically return an empty Optional only if the cron was specified to run for a given year/set of years and the event had run for the last time in the last year.

    @Test
    public void emptyNextTimeCron() {
        final JobTriggerDto trigger = JobTriggerDto.builderWithClock(clock)
                .jobDefinitionId("abc-123")
                .jobDefinitionType("event-processor-execution-v1")
                .schedule(CronJobSchedule.builder()
                        .cronExpression("0 0 * ? * * 2024")
                        .build())
                .build();

        final DateTime nye2024 = DateTime.parse("2024-12-31T23:00:00.000Z");
        final Optional<DateTime> nextTime = strategies.nextTime(trigger, nye2024);

        assertThat(nextTime).isEmpty();
    }

In which case, the nextTime value would also be empty because the trigger's lastNextTime would have to be after the last to time. So the event would not run.

@kingzacko1 kingzacko1 force-pushed the 19365-add-option-to-disableenable-alerts-for-certain-time-intervals branch from d1b7479 to 81bc76b Compare June 11, 2024 18:31
@kingzacko1 kingzacko1 requested a review from zeeklop June 11, 2024 18:34
Copy link
Contributor

@danotorrey danotorrey left a comment

Choose a reason for hiding this comment

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

@kingzacko1 I ran through this today and did some testing. It's looking awesome! It worked well with my tests and jobs always seemed to execute per the cron schedule. The frontend is working well too in terms of validating the expression on save, and the validate API endpoint was super-helpful too. I'll do a bit more testing on the job catch up logic and will report back when done.

Copy link
Contributor

@danotorrey danotorrey left a comment

Choose a reason for hiding this comment

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

@kingzacko1 LGTM and tests successfully. Frontend editing of events with and without cron schedules works successfully, and cron schedules execute successfully.

Copy link
Contributor

@ryan-carroll-graylog ryan-carroll-graylog left a comment

Choose a reason for hiding this comment

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

Looks great and functionality wise all tests successfully!

Had one nit about possible error UI massaging, but otherwise this looks good to go to me!

@kingzacko1 kingzacko1 requested a review from bernd June 12, 2024 16:36
@zeeklop
Copy link
Contributor

zeeklop commented Jun 12, 2024

The modification tested successfully. Thank you @kingzacko1 for getting the front end done as well. The one thing that is odd for me, is the placement of the checkbox and the fields.

So when the user selects the checkbox to set a cron schedule, the fields change above the checkbox. I believe the checkbox should be placed on top of the fields. It would match better the vertical flow of the form.

zeeklop
zeeklop approved these changes Jun 14, 2024
@zeeklop
Copy link
Contributor

zeeklop commented Jun 14, 2024

This looks slick! nice work! tested successfully. I notice one thing, pressing the sync button disables the sync schedule, is that behavior correct?

Sorry. this was meant for Simon's PR. Got my tabs crossed

@zeeklop zeeklop self-requested a review June 14, 2024 18:39
…-disableenable-alerts-for-certain-time-intervals
Copy link
Contributor

@zeeklop zeeklop left a comment

Choose a reason for hiding this comment

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

Tested successfully. Thank you Zack!

Copy link
Contributor

@danotorrey danotorrey left a comment

Choose a reason for hiding this comment

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

LGTM and tests successfully with the latest.

@kingzacko1 kingzacko1 merged commit b6a52c7 into master Jun 27, 2024
6 checks passed
@kingzacko1 kingzacko1 deleted the 19365-add-option-to-disableenable-alerts-for-certain-time-intervals branch June 27, 2024 14:02
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.

Add option to disable/enable alerts for certain time intervals
6 participants