feat: add support for scheduling functions #1527

rtpascual · 2024-05-16T01:01:48Z

Changes

Add the ability to schedule functions:

Natural language - starts with every then the rest is based on the unit of time
- Minutes and Hours
  - every followed by a space then a positive whole number and either m for minute(s) or h for hour(s)
  - Examples: every 5m or every 1h
- Days, Weeks, Months, Years
  - every followed by a space then either day, week, month, or year
  - Days will always start at 00:00
  - Weeks start on Sundays at 00:00
  - Months start on 1st day of the month at 00:00
  - Years start Jan 1st at 00:00
  - Examples: every day or every year
Cron expression - 5 or 6 cron "parts" (year is not required) which follow Amazon EventBridge cron expressions for possible values
- What is not supported in this implementation:
  - JAN-DEC for months
  - SUN-SAT for days of the week
  - The L wildcard
  - The # wildcard
  - The W wildcard
An array that contains either of the above.

export const everyFiveMinutes = defineFunction({
    name: 'everyFiveMinutes',
    schedule: 'every 5m',
});

export const everyHour = defineFunction({
    name: 'everyHour',
    schedule: 'every 1h',
});

export const cronFiveThirtyDaily = defineFunction({
    name: 'cronFiveThirtyDaily',
    schedule: '30 17 * * *',
});

export const cronWednesdayAtNine = defineFunction({
    name: 'cronWednesdayAtNine',
    schedule: '0 9 * * 3',
});

export const arrayEvery = defineFunction({
    name: 'arrayEvery',
    schedule: [
        'every 4h',
        'every 40m'
    ],
});

export const arrayCron = defineFunction({
    name: 'arrayCron',
    schedule: [
        '25 * * * *',
        '* 3 * * *',
        '0 1 * * * *',
    ],
});

export const arrayMixed = defineFunction({
    name: 'arrayMixed',
    schedule: [
        'every 6h',
        '* 5 * * *',
    ],
});

Validation

Deployed with test project and observed lambdas with EventBridge rules
Unit and E2E tests

Checklist

If this PR includes a functional change to the runtime behavior of the code, I have added or updated automated test coverage for this change.
If this PR requires a change to the Project Architecture README, I have included that update in this PR.
If this PR requires a docs update, I have linked to that docs PR above.
If this PR modifies E2E tests, makes changes to resource provisioning, or makes SDK calls, I have run the PR checks with the run-e2e label set.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

changeset-bot · 2024-05-16T01:01:51Z

🦋 Changeset detected

Latest commit: ca2d0bd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@aws-amplify/backend-function	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

rtpascual · 2024-05-17T23:52:39Z

packages/backend-function/src/factory.ts

+export type Cron =
+  | `${string} ${string} ${string} ${string} ${string}`
+  | `${string} ${string} ${string} ${string} ${string} ${string}`;
+export type Rate =
+  | `every ${number}m`
+  | `every ${number}h`
+  | `every day`
+  | `every week`
+  | `every month`
+  | `every year`;
+export type TimeInterval = Rate | Cron;


Types were done this way because I started getting Expression produces a union type that is too complex to represent Typescript error when I introduced wildcards for cron. Cron type is less strict and we perform our own cron validation to validate what we support (which could change in the future).

packages/backend-function/src/factory.ts

packages/backend-function/API.md

…its timeout

edwardfoyle · 2024-07-11T17:59:58Z

packages/backend-function/src/schedule_parser.ts

+        const { value, unit } = parseRate(interval);
+
+        if (value && !isPositiveWholeNumber(value)) {
+          throw new Error(`schedule must be set with a positive whole number`);


AmplifyUserError

edwardfoyle · 2024-07-11T18:00:33Z

packages/backend-function/src/schedule_parser.ts

+        ) {
+          const timeout = lambda.timeout.toSeconds();
+
+          throw new Error(


AmplifyUserError

edwardfoyle · 2024-07-11T18:01:28Z

packages/backend-function/src/schedule_parser.ts

+          const timeout = lambda.timeout.toSeconds();
+
+          throw new Error(
+            `schedule must be greater than the timeout of ${timeout} ${


Suggested change

`schedule must be greater than the timeout of ${timeout} ${

`Function schedule rate must be greater than the function timeout of ${timeout} ${

edwardfoyle · 2024-07-11T18:03:30Z

packages/backend-function/src/schedule_parser.ts

+        }
+      } else {
+        if (!isValidCron(interval)) {
+          // TODO: Better error messaging here or throw within each part of isValidCron in order to give more concise error messages


This is a good callout, I think we should go ahead and do this. My personal preference is to collect all of the validation errors into one object and then throw a single error that explains all of the validation violations. This avoids fixing one thing, getting another error, fixing that, getting another error, etc.

edwardfoyle · 2024-07-11T18:04:11Z

packages/backend-function/src/schedule_parser.ts

+          schedule: Schedule.cron(translateToCronOptions(interval)),
+        });
+
+        rule.addTarget(new targets.LambdaFunction(this.lambda));


do we need to create a new target for each rule or can we reuse the same target since they are all pointing to the same lambda?

Good catch, I believe we should be able to create the target once and reuse for each rule.

edwardfoyle · 2024-07-11T18:06:24Z

packages/backend-function/src/schedule_parser.ts

+  /**
+   * Initialize EventBridge rules
+   */
+  constructor(


non-blocker: since this class is purely internal it's not a big deal, but this is a lot of logic for a ctor. I think this is a case where we may not even need a class and just a convertTimeIntervalsToEventRules function would suffice for this functionality.

I also think the caller should be responsible for attaching the event rule to the lambda rather than it happening internally here. It's a small single-responsibility violation.

edwardfoyle · 2024-07-11T18:10:13Z

packages/backend-function/src/schedule_parser.ts

+        };
+      default:
+        // This should never happen with strict types
+        throw new Error('Could not determine the schedule for the function');


AmplifyUserError?

edwardfoyle · 2024-07-11T18:17:43Z

packages/integration-tests/src/test-project-setup/data_storage_auth_with_triggers.ts

@@ -228,6 +235,7 @@ class DataStorageAuthWithTriggerTestProject extends TestProjectBase {
    await this.checkLambdaResponse(node16Lambda[0], expectedResponse);
    await this.checkLambdaResponse(funcWithSsm[0], 'It is working');
    await this.checkLambdaResponse(funcWithAwsSdk[0], 'It is working');
+    await this.checkLambdaResponse(funcWithSchedule[0], 'It is working');


technically this isn't checking that the event trigger is working, just that the lambda can be invoked successfully. I'm not sure how worthwhile it is, but a more comprehensive test might be to create a lambda that sets a value in parameter store (or leaves some trace of execution), set an "every second" schedule for that function, wait a couple seconds after the deployment completes, then check for the execution breadcrumb.

Or maybe a simpler test is to check that the lambda can be invoked (ie what you have here) + check cloudtrail for the invocation event

EventBridge has a minimum of 1 minute for rule schedules so we would have to wait for ~1 minute for the event to invoke the lambda.

I was thinking the unit tests checking the event rule is properly added (which I just realized I should also make sure the rule target is properly set) and this E2E test is enough to have confidence this will work. I can explore the 2nd option you suggested.

I don't see a way around fully testing this without having the test wait the minimum time it takes for the event to trigger.

Still trying different ways to reduce flakiness with current E2E implementation.

I think the 70 second wait you added is okay. If it proves to be slow / flaky we can reevaluate the strategy and value of the test. Thanks for investigating this!

Thanks! Yeah I was playing with the time considering EventBridge may have a delay of several seconds so 70 seconds seemed like a good number with the minimum time of 1 minute for schedules.

Amplifiyer · 2024-07-12T12:02:16Z

packages/backend-function/API.md

 };

 // @public (undocumented)
 export type NodeVersion = 16 | 18 | 20;

+// @public (undocumented)
+export type Rate = `every ${number}m` | `every ${number}h` | `every day` | `every week` | `every month` | `every year`;


Suggestion for alternative namings:

type TimeInterval = `every xx`; type CronSchedule = `{string} ...` type Schedule = TimeInterval | CronSchedule

I was avoiding type Schedule because of the naming clash with the Schedule class from aws-cdk-lib/aws-events. Alternatively we can use FunctionSchedule?

Amplifiyer · 2024-07-12T12:12:44Z

packages/backend-function/src/schedule_parser.ts

+      } else {
+        if (!isValidCron(interval)) {
+          // TODO: Better error messaging here or throw within each part of isValidCron in order to give more concise error messages
+          throw new Error(`schedule cron expression is not valid`);


The message may not be always true, e.g. if I say every 5 months it will throw invalid cron expression but I specified an interval.

With strict typing and the more exhaustive validation you mentioned below, I believe everything that gets to this point will be validation for cron expressions

Amplifiyer · 2024-07-12T12:16:20Z

packages/backend-function/src/schedule_parser.ts

+}
+
+const isRate = (timeInterval: TimeInterval): timeInterval is Rate => {
+  return timeInterval.split(' ')[0] === 'every';


More exhaustive validation

Suggested change

return timeInterval.split(' ')[0] === 'every';

const parts = timeInterval.split(' ');

return parts[0] === 'every' && (['m', 'h', 'day', 'week', 'year', 'month'].some(a => parts[1].endsWith(a))) && parts.length === 2

Because customers could be using this in a JS only app or have TS errors suppressed in their files or just creative enough to pass every 5 weeks until 2025 (Which our type would allow thinking it's a cron)

We can also get rid of it by just using tryParseRate.

Amplifiyer · 2024-07-12T12:29:17Z

packages/backend-function/src/schedule_parser.ts

+  const regex = /\d/;
+  if (interval.match(regex)) {


Would this not match just one digit? Ideally we should parse with a more confidence that it is indeed a rate. E.g. with regex ^/\d+[mh]$|^day|year|month$/

Yes, my goal was to just see if there is a number in the second part of the "rate string" in order to get the number value and unit (m or h). Otherwise the second part would be day, week, etc.

The Rate type is doing most of the heavy lifting when I initially made the validation (which I'll revisit based on your previous point on customers using JS only or have TS errors suppressed)

edwardfoyle

LGTM, just one naming nit if you want to take it

packages/backend-function/src/factory.ts

Co-authored-by: Edward Foyle <[email protected]>

edwardfoyle

LGTM, couple nits if you want to address

packages/backend-function/src/schedule_parser.ts

edwardfoyle · 2024-07-17T21:19:51Z

packages/integration-tests/src/test-project-setup/data_storage_auth_with_triggers.ts

-    await this.checkLambdaResponse(funcWithSchedule[0], 'It is working');
+
+    // test schedule function event trigger once
+    if (this.checkInvocationCount) {


I'm assuming this is here to prevent duplicate checking, but I'm confused why it's necessary?

This reduced a lot of flakiness that I was seeing by only testing for invocation count on the first deployment instead of also on subsequent sandbox updates. This will also help us reduce time on e2e tests as we don't have to wait 70 seconds for each assertPostDeployment.

Sounds good. Could be helpful to leave that context in a comment on this code.

Done in ca2d0bd

edwardfoyle

LGTM. Take or leave this comment: #1527 (comment)

packages/backend-function/src/factory.ts

feat: add support for scheduling functions

acec70c

update valid natural language and cron

a5d6533

rtpascual commented May 17, 2024

View reviewed changes

Merge branch 'main' into schedule-functions

afbd6f3

josefaidt linked an issue May 19, 2024 that may be closed by this pull request

Gen2: Scheduled Lambdas are not possible anymore #1491

Closed

3 tasks

edwardfoyle reviewed May 20, 2024

View reviewed changes

packages/backend-function/src/factory.ts Show resolved Hide resolved

edwardfoyle reviewed May 20, 2024

View reviewed changes

packages/backend-function/API.md Outdated Show resolved Hide resolved

rtpascual added 5 commits June 11, 2024 13:04

Merge branch 'main' into schedule-functions

f104c9a

pr feedback and add unit tests

4346570

add test that throws when schedule will invoke function again before …

d7f26db

…its timeout

update schedule rate validation

562e275

Merge branch 'main' into schedule-functions

255dfc5

rtpascual marked this pull request as ready for review July 9, 2024 22:09

rtpascual requested review from a team as code owners July 9, 2024 22:09

rtpascual added 2 commits July 10, 2024 09:36

Merge branch 'main' into schedule-functions

46d8d08

add tests

3e0bccd

rtpascual added the run-e2e Label that will include e2e tests in PR checks workflow label Jul 10, 2024

rtpascual added 2 commits July 10, 2024 15:09

update e2e test

4f797ec

fix e2e test

d86ea22

edwardfoyle reviewed Jul 11, 2024

View reviewed changes

Amplifiyer reviewed Jul 12, 2024

View reviewed changes

rtpascual added 7 commits July 15, 2024 14:30

PR feedback

8e8063f

Merge branch 'main' into schedule-functions

760888a

fix test

79dfa9f

add more testing

2d11214

fix lint and e2e test

3c846fb

try this

f6821ba

try this

f0311eb

rtpascual added 3 commits July 16, 2024 14:42

try that

0d7e55a

try this

6b89ceb

Merge branch 'main' into schedule-functions

46a2192

edwardfoyle previously approved these changes Jul 16, 2024

View reviewed changes

packages/backend-function/src/factory.ts Outdated Show resolved Hide resolved

Update packages/backend-function/src/factory.ts

2950c72

Co-authored-by: Edward Foyle <[email protected]>

rtpascual dismissed edwardfoyle’s stale review via 2950c72 July 17, 2024 00:04

edwardfoyle previously approved these changes Jul 17, 2024

View reviewed changes

rtpascual added 2 commits July 17, 2024 14:50

Merge branch 'main' into schedule-functions

0fa4a84

pr feedback

28a6825

rtpascual dismissed edwardfoyle’s stale review via 28a6825 July 17, 2024 22:02

edwardfoyle previously approved these changes Jul 18, 2024

View reviewed changes

awsluja reviewed Jul 18, 2024

View reviewed changes

packages/backend-function/src/factory.ts Outdated Show resolved Hide resolved

pr feedback

ca2d0bd

rtpascual dismissed edwardfoyle’s stale review via ca2d0bd July 18, 2024 21:54

awsluja approved these changes Jul 18, 2024

View reviewed changes

edwardfoyle approved these changes Jul 19, 2024

View reviewed changes

rtpascual merged commit ec12ac8 into main Jul 19, 2024
53 checks passed

rtpascual deleted the schedule-functions branch July 19, 2024 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for scheduling functions #1527

feat: add support for scheduling functions #1527

rtpascual commented May 16, 2024 •

edited

Loading

changeset-bot bot commented May 16, 2024 •

edited

Loading

rtpascual May 17, 2024

edwardfoyle Jul 11, 2024

edwardfoyle Jul 11, 2024

edwardfoyle Jul 11, 2024

edwardfoyle Jul 11, 2024

edwardfoyle Jul 11, 2024

rtpascual Jul 11, 2024

edwardfoyle Jul 11, 2024

edwardfoyle Jul 11, 2024

edwardfoyle Jul 11, 2024

rtpascual Jul 11, 2024

rtpascual Jul 16, 2024

edwardfoyle Jul 16, 2024 •

edited

Loading

rtpascual Jul 17, 2024

Amplifiyer Jul 12, 2024

rtpascual Jul 15, 2024

Amplifiyer Jul 12, 2024

rtpascual Jul 15, 2024

Amplifiyer Jul 12, 2024

Amplifiyer Jul 12, 2024

Amplifiyer Jul 12, 2024

rtpascual Jul 15, 2024

edwardfoyle left a comment

edwardfoyle left a comment

edwardfoyle Jul 17, 2024

rtpascual Jul 17, 2024

edwardfoyle Jul 18, 2024

rtpascual Jul 18, 2024

edwardfoyle left a comment

	`schedule must be greater than the timeout of ${timeout} ${
	`Function schedule rate must be greater than the function timeout of ${timeout} ${

	return timeInterval.split(' ')[0] === 'every';
	const parts = timeInterval.split(' ');
	return parts[0] === 'every' && (['m', 'h', 'day', 'week', 'year', 'month'].some(a => parts[1].endsWith(a))) && parts.length === 2

feat: add support for scheduling functions #1527

feat: add support for scheduling functions #1527

Conversation

rtpascual commented May 16, 2024 • edited Loading

Changes

Validation

Checklist

changeset-bot bot commented May 16, 2024 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edwardfoyle Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edwardfoyle left a comment

Choose a reason for hiding this comment

edwardfoyle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edwardfoyle left a comment

Choose a reason for hiding this comment

rtpascual commented May 16, 2024 •

edited

Loading

changeset-bot bot commented May 16, 2024 •

edited

Loading

edwardfoyle Jul 16, 2024 •

edited

Loading