Repository: elastic/synthetics (ts)

Issue:
add validation for monitor schedule for project monitors
+ Goes in sync with https://github.com/elastic/kibana/issues/142655
+ Couple of things we need to do on the agent side to prevent the unsupported scheduling
    - Improve the typings for browser monitors `schedule` in `monitor.use` DSL and synthetics config file. Browser allows 1, 3, 5, 10, 15, 30 and 60 minutes. 
    - Round off the scheduling for lightweight monitors to the nearest whole minute  (1 minute if the schedule is less than 60 seconds). Double check if the current logic needs any changes other than changing the warning message to `If you are using lightweight monitors with less than 1 minute resolution, these will be saved to the nearest supported frequency`

Pull request:
feat: improve validation for monitor schedules
+ fix #620 
+ PR Adds couple of things to make the schedule validation intact for lightweight and browser monitors. 
+ When users scafolds the project using `init` command, we default to showing the available schedule choices based on the browser schedule instead of defaulting to 10, Keeping the initial value still at 10. 
<img width="562" alt="Screen Shot 2022-10-05 at 2 35 40 PM" src="https://user-images.githubusercontent.com/3902525/194168481-9e1b6b49-7200-41aa-ab54-d9920534c5bc.png">

+ Validates the `synthetics.config.ts` file for schedule if its not using the default available schedules - This helps to prevent users who are already using custom schedules to move towards the available ones once they have upgraded to the new version of the agent. 
<img width="707" alt="Screen Shot 2022-10-05 at 2 38 12 PM" src="https://user-images.githubusercontent.com/3902525/194168822-c5c8e46e-aa9e-4828-abd1-d9a38db8a8be.png">

+  Improve the typings of the `monitor.use({ schedule: 10 })` to only accept the acceptable browser values. 
+  Throws validation error when users are using any other invalid values that is not part of the acceptable list. 
<img width="839" alt="Screen Shot 2022-10-05 at 2 40 05 PM" src="https://user-images.githubusercontent.com/3902525/194169085-f19a0936-b841-4ecf-aa65-35631a1b0432.png">

+  Added warning for lightweight monitor schedules only if we see a valid heartbeat monitor configuration and these schedules are rounded off to the nearest minute values. 

### Testing
1. Build the local project using - npm run build
2. Run npm link
3. Setup a brand new Synthetics project - node dist/cli.js init [optional-dir]
4. Navigate to the new directory created with init and run npm link @elastic/synthetics
5. Follow along and check if project settings have been initialized properly
6. Try going through the above scenarios with `npm run push` command. 

Patch:
diff --git a/src/core/runner.ts b/src/core/runner.ts
index 66aa6eb..770ced0 100644
--- a/src/core/runner.ts
+++ b/src/core/runner.ts
@@ -412,6 +412,7 @@ export default class Runner {
        */
       journey.callback({ params: options.params } as any);
       journey.monitor.update(this.monitor?.config);
+      journey.monitor.validate();
       monitors.push(journey.monitor);
     }
     return monitors;
diff --git a/src/dsl/monitor.ts b/src/dsl/monitor.ts
index 47e3aeb..510b017 100644
--- a/src/dsl/monitor.ts
+++ b/src/dsl/monitor.ts
@@ -24,6 +24,7 @@
  */
 
 import merge from 'deepmerge';
+import { bold, red } from 'kleur/colors';
 import {
   ThrottlingOptions,
   Location,
@@ -31,19 +32,21 @@ import {
   Params,
   PlaywrightOptions,
 } from '../common_types';
+import { indent } from '../helpers';
 import { LocationsMap } from '../locations/public-locations';
 
 export type SyntheticsLocationsType = keyof typeof LocationsMap;
 export const SyntheticsLocations = Object.keys(
   LocationsMap
 ) as SyntheticsLocationsType[];
+export const ALLOWED_SCHEDULES = [1, 3, 5, 10, 15, 30, 60] as const;
 
 export type MonitorConfig = {
   id?: string;
   name?: string;
   type?: string;
   tags?: string[];
-  schedule?: number;
+  schedule?: typeof ALLOWED_SCHEDULES[number];
   enabled?: boolean;
   locations?: SyntheticsLocationsType[];
   privateLocations?: string[];
@@ -90,4 +93,22 @@ export class Monitor {
   setFilter(filter: MonitorFilter) {
     this.filter = filter;
   }
+
+  validate() {
+    const schedule = this.config.schedule;
+    if (ALLOWED_SCHEDULES.includes(schedule)) {
+      return;
+    }
+    const { config, source } = this;
+    let outer = bold(
+      `Invalid schedule: ${schedule}, allowed values are ${ALLOWED_SCHEDULES.join(
+        ','
+      )}\n`
+    );
+    if (source) {
+      const inner = `* ${config.id} - ${source.file}:${source.line}:${source.column}\n`;
+      outer += indent(inner);
+    }
+    throw red(outer);
+  }
 }
diff --git a/src/generator/index.ts b/src/generator/index.ts
index a5a575c..4228867 100644
--- a/src/generator/index.ts
+++ b/src/generator/index.ts
@@ -37,6 +37,7 @@ import {
   cloudIDToKibanaURL,
 } from './utils';
 import { formatLocations, getLocations, groupLocations } from '../locations';
+import { ALLOWED_SCHEDULES } from '../dsl/monitor';
 import type { ProjectSettings } from '../common_types';
 
 // Templates that are required for setting up new synthetics project
@@ -133,10 +134,15 @@ export class Generator {
         },
       },
       {
-        type: 'numeral',
+        type: 'select',
         name: 'schedule',
         message: 'Set default schedule in minutes for all monitors',
-        initial: 10,
+        initial: 3, // Index of the third array item which is 10 minutes
+        choices: ALLOWED_SCHEDULES.map(String),
+        required: true,
+        result(value) {
+          return Number(value) as any;
+        },
       },
       {
         type: 'input',
diff --git a/src/push/index.ts b/src/push/index.ts
index 4da51e0..048753f 100644
--- a/src/push/index.ts
+++ b/src/push/index.ts
@@ -34,7 +34,7 @@ import {
   formatStaleMonitors,
 } from './request';
 import { buildMonitorSchema, createMonitors, MonitorSchema } from './monitor';
-import { Monitor } from '../dsl/monitor';
+import { ALLOWED_SCHEDULES, Monitor } from '../dsl/monitor';
 import {
   progress,
   apiProgress,
@@ -192,6 +192,10 @@ export function validateSettings(opts: PushOptions) {
     reason = `Set default schedule in minutes for all monitors via
   - CLI '--schedule <mins>'
   - Config file 'monitors.schedule' field`;
+  } else if (opts.schedule && !ALLOWED_SCHEDULES.includes(opts.schedule)) {
+    reason = `Set default schedule(${
+      opts.schedule
+    }) to one of the allowed values - ${ALLOWED_SCHEDULES.join(',')}`;
   }
 
   if (!reason) return;
diff --git a/src/push/monitor.ts b/src/push/monitor.ts
index 052f86a..64155a0 100644
--- a/src/push/monitor.ts
+++ b/src/push/monitor.ts
@@ -37,7 +37,7 @@ import {
   warn,
 } from '../helpers';
 import { LocationsMap } from '../locations/public-locations';
-import { Monitor, MonitorConfig } from '../dsl/monitor';
+import { ALLOWED_SCHEDULES, Monitor, MonitorConfig } from '../dsl/monitor';
 import { PushOptions } from '../common_types';
 
 export type MonitorSchema = Omit<MonitorConfig, 'locations'> & {
@@ -95,13 +95,8 @@ export async function createLightweightMonitors(
       lwFiles.add(abs);
     }
   });
-  // Warn users about schedule that are less than 60 seconds
-  if (lwFiles.size > 0) {
-    warn(
-      'lightweight monitors schedule < 1 minute resolution are not supported, will get reset to their nearest minute interval.'
-    );
-  }
 
+  let warnOnce = false;
   const monitors: Monitor[] = [];
   for (const file of lwFiles.values()) {
     const content = await readFile(file, 'utf-8');
@@ -115,6 +110,13 @@ export async function createLightweightMonitors(
     if (!monitorSeq) {
       continue;
     }
+    // Warn users about schedule that are less than 60 seconds
+    if (!warnOnce) {
+      warn(
+        'If you are using lightweight monitors with less than 1 minute resolution, these will be saved to the nearest supported frequency'
+      );
+      warnOnce = true;
+    }
 
     for (const monNode of monitorSeq.items) {
       // Skip browser monitors and disabled monitors from pushing
@@ -193,7 +195,26 @@ export function parseSchedule(schedule: string) {
         break;
     }
   }
-  return minutes;
+  return nearestSchedule(minutes);
+}
+
+// Find the nearest schedule that is supported by the platform
+// from the parsed schedule value
+export function nearestSchedule(schedule: number) {
+  let start = 0;
+  let end = ALLOWED_SCHEDULES.length - 1;
+  while (start <= end) {
+    const mid = Math.floor((start + end) / 2);
+    const midValue = ALLOWED_SCHEDULES[mid];
+    if (midValue === schedule) {
+      return midValue;
+    } else if (midValue < schedule) {
+      start = mid + 1;
+    } else {
+      end = mid - 1;
+    }
+  }
+  return ALLOWED_SCHEDULES[end];
 }
 
 export async function createMonitors(


Test patch:
diff --git a/__tests__/core/runner.test.ts b/__tests__/core/runner.test.ts
index 41fc56f..c069e5a 100644
--- a/__tests__/core/runner.test.ts
+++ b/__tests__/core/runner.test.ts
@@ -712,10 +712,10 @@ describe('runner', () => {
     const j2 = new Journey({ name: 'j2' }, noop);
     j1.updateMonitor({
       id: 'test-j1',
-      schedule: 2,
+      schedule: 3,
       locations: ['united_kingdom'],
     });
-    j2.updateMonitor({ throttling: { latency: 1000 } });
+    j2.updateMonitor({ throttling: { latency: 1000 }, schedule: 1 });
     runner.addJourney(j1);
     runner.addJourney(j2);
 
@@ -730,7 +730,7 @@ describe('runner', () => {
       tags: [],
       locations: ['united_kingdom'],
       privaateLocations: undefined,
-      schedule: 2,
+      schedule: 3,
       params: undefined,
       playwrightOptions: undefined,
       throttling: { download: 5, latency: 20, upload: 3 },
@@ -754,7 +754,7 @@ describe('runner', () => {
     const j2 = new Journey({ name: 'j2' }, noop);
     j1.updateMonitor({
       id: 'test-j1',
-      schedule: 2,
+      schedule: 3,
       locations: ['united_kingdom'],
       privateLocations: ['spain'],
     });
@@ -773,7 +773,7 @@ describe('runner', () => {
       tags: ['foo*'],
       locations: ['united_kingdom'],
       privateLocations: ['spain'],
-      schedule: 2,
+      schedule: 3,
       params: { env: 'test' },
       playwrightOptions: { ignoreHTTPSErrors: true },
       throttling: { download: 100, latency: 20, upload: 50 },
@@ -792,7 +792,7 @@ describe('runner', () => {
     runner.addJourney(j1);
     runner.addJourney(j2);
 
-    const monitors = runner.buildMonitors({ match: 'j1' });
+    const monitors = runner.buildMonitors({ match: 'j1', schedule: 1 });
     expect(monitors.length).toBe(1);
     expect(monitors[0].config.name).toBe('j1');
   });
@@ -805,7 +805,7 @@ describe('runner', () => {
     runner.addJourney(j2);
     runner.addJourney(j3);
 
-    const monitors = runner.buildMonitors({ tags: ['first'] });
+    const monitors = runner.buildMonitors({ tags: ['first'], schedule: 1 });
     expect(monitors.length).toBe(1);
     expect(monitors[0].config.name).toBe('j1');
   });
diff --git a/__tests__/options.test.ts b/__tests__/options.test.ts
index 93b633b..c81aadf 100644
--- a/__tests__/options.test.ts
+++ b/__tests__/options.test.ts
@@ -79,9 +79,9 @@ describe('options', () => {
     });
 
     expect(
-      normalizeOptions({ throttling: { download: 50 }, schedule: 2 })
+      normalizeOptions({ throttling: { download: 50 }, schedule: 3 })
     ).toMatchObject({
-      schedule: 2,
+      schedule: 3,
       throttling: {
         download: 50,
         upload: 3,
diff --git a/__tests__/push/__snapshots__/index.test.ts.snap b/__tests__/push/__snapshots__/index.test.ts.snap
index 5376a15..5eadd99 100644
--- a/__tests__/push/__snapshots__/index.test.ts.snap
+++ b/__tests__/push/__snapshots__/index.test.ts.snap
@@ -43,6 +43,15 @@ Run 'npx @elastic/synthetics init' to create project with default settings.
 exports[`Push error on invalid schedule 1`] = `
 "Aborted. Invalid synthetics project settings.
 
+Set default schedule(12) to one of the allowed values - 1,3,5,10,15,30,60
+
+Run 'npx @elastic/synthetics init' to create project with default settings.
+"
+`;
+
+exports[`Push error when schedule is not present 1`] = `
+"Aborted. Invalid synthetics project settings.
+
 Set default schedule in minutes for all monitors via
   - CLI '--schedule <mins>'
   - Config file 'monitors.schedule' field
diff --git a/__tests__/push/index.test.ts b/__tests__/push/index.test.ts
index 13055f6..12ab12b 100644
--- a/__tests__/push/index.test.ts
+++ b/__tests__/push/index.test.ts
@@ -85,16 +85,25 @@ describe('Push', () => {
     expect(output).toMatchSnapshot();
   });
 
-  it('error on invalid schedule', async () => {
+  it('error when schedule is not present', async () => {
     await fakeProjectSetup({ id: 'test-project' }, { locations: ['test-loc'] });
     const output = await runPush();
     expect(output).toMatchSnapshot();
   });
 
+  it('error on invalid schedule', async () => {
+    await fakeProjectSetup(
+      { id: 'test-project' },
+      { locations: ['test-loc'], schedule: 12 }
+    );
+    const output = await runPush();
+    expect(output).toMatchSnapshot();
+  });
+
   it('abort on push with different project id', async () => {
     await fakeProjectSetup(
       { id: 'test-project' },
-      { locations: ['test-loc'], schedule: 2 }
+      { locations: ['test-loc'], schedule: 3 }
     );
     const output = await runPush(
       [...DEFAULT_ARGS, '-y', '--id', 'new-project'],
@@ -105,15 +114,31 @@ describe('Push', () => {
     expect(output).toContain('Push command Aborted');
   });
 
+  it('error on invalid schedule in monitor DSL', async () => {
+    await fakeProjectSetup(
+      { id: 'test-project', space: 'dummy', url: 'http://localhost:8080' },
+      { locations: ['test-loc'], schedule: 3 }
+    );
+    const testJourney = join(PROJECT_DIR, 'test.journey.ts');
+    await writeFile(
+      testJourney,
+      `import {journey, monitor} from '../../../src';
+journey('journey 1', () => monitor.use({ id: 'j1', schedule: 8 }));`
+    );
+    const output = await runPush();
+    expect(output).toContain('Invalid schedule: 8, allowed values are');
+    await rm(testJourney, { force: true });
+  });
+
   it('push with different id when overriden', async () => {
     await fakeProjectSetup(
       { id: 'test-project', space: 'dummy', url: 'http://localhost:8080' },
-      { locations: ['test-loc'], schedule: 2 }
+      { locations: ['test-loc'], schedule: 3 }
     );
     const testJourney = join(PROJECT_DIR, 'test.journey.ts');
     await writeFile(
       testJourney,
-      `import {journey, monitor} from '../../../src/index';
+      `import {journey, monitor} from '../../../src';
 journey('journey 1', () => monitor.use({ id: 'j1' }));`
     );
     const output = await runPush(
@@ -129,7 +154,7 @@ journey('journey 1', () => monitor.use({ id: 'j1' }));`
   it('errors on duplicate browser monitors', async () => {
     await fakeProjectSetup(
       { id: 'test-project' },
-      { locations: ['test-loc'], schedule: 2 }
+      { locations: ['test-loc'], schedule: 3 }
     );
 
     const dupJourney = join(PROJECT_DIR, 'duplicate.journey.ts');
@@ -140,7 +165,7 @@ journey('journey 1', () => monitor.use({ id: 'duplicate id' }));
 journey('journey 2', () => monitor.use({ id: 'duplicate id' }));
 
 journey('duplicate name', () => monitor.use({ schedule: 10 }));
-journey('duplicate name', () => monitor.use({ schedule: 20 }));`
+journey('duplicate name', () => monitor.use({ schedule: 15 }));`
     );
     const output = await runPush();
     expect(output).toContain(`Aborted: Duplicate monitors found`);
@@ -152,7 +177,7 @@ journey('duplicate name', () => monitor.use({ schedule: 20 }));`
   it('errors on duplicate lightweight monitors', async () => {
     await fakeProjectSetup(
       { id: 'test-project' },
-      { locations: ['test-loc'], schedule: 2 }
+      { locations: ['test-loc'], schedule: 3 }
     );
 
     const heartbeatYml = join(PROJECT_DIR, 'hearbeat.duplicate.yml');
@@ -192,7 +217,7 @@ heartbeat.monitors:
   it('abort when delete is skipped', async () => {
     await fakeProjectSetup(
       { id: 'test-project' },
-      { locations: ['test-loc'], schedule: 2 }
+      { locations: ['test-loc'], schedule: 3 }
     );
     const output = await runPush([...DEFAULT_ARGS, '-y'], {
       TEST_OVERRIDE: false,
@@ -203,7 +228,7 @@ heartbeat.monitors:
   it('delete entire project with --yes flag', async () => {
     await fakeProjectSetup(
       { id: 'test-project', space: 'dummy', url: 'http://localhost:8080' },
-      { locations: ['test-loc'], schedule: 2 }
+      { locations: ['test-loc'], schedule: 3 }
     );
     const output = await runPush([...DEFAULT_ARGS, '-y']);
     expect(output).toContain('deleting all stale monitors');
@@ -212,7 +237,7 @@ heartbeat.monitors:
   it('delete entire project with overrides', async () => {
     await fakeProjectSetup(
       { id: 'test-project', space: 'dummy', url: 'http://localhost:8080' },
-      { locations: ['test-loc'], schedule: 2 }
+      { locations: ['test-loc'], schedule: 3 }
     );
     const output = await runPush([...DEFAULT_ARGS, '-y'], {
       TEST_OVERRIDE: true,
@@ -249,7 +274,7 @@ heartbeat.monitors:
           id: 'test-project',
           space: 'dummy',
         },
-        { locations: ['test-loc'], schedule: 2 }
+        { locations: ['test-loc'], schedule: 3 }
       );
 
       await writeFile(
diff --git a/__tests__/push/monitor.test.ts b/__tests__/push/monitor.test.ts
index e4b6fdc..5f23143 100644
--- a/__tests__/push/monitor.test.ts
+++ b/__tests__/push/monitor.test.ts
@@ -85,14 +85,16 @@ describe('Monitors', () => {
       `Monitor schedule format(* * * *) not supported: use '@every' syntax instead`
     );
     expect(parseSchedule('@every 4s')).toBe(1);
-    expect(parseSchedule('@every 60s')).toBe(1);
-    expect(parseSchedule('@every 70s')).toBe(2);
+    expect(parseSchedule('@every 70s')).toBe(1);
     expect(parseSchedule('@every 121s')).toBe(3);
     expect(parseSchedule('@every 1m')).toBe(1);
-    expect(parseSchedule('@every 1m10s')).toBe(2);
-    expect(parseSchedule('@every 2m')).toBe(2);
-    expect(parseSchedule('@every 1h2m')).toBe(62);
-    expect(parseSchedule('@every 1h2m10s')).toBe(63);
+    expect(parseSchedule('@every 2m10s')).toBe(3);
+    expect(parseSchedule('@every 4m25s')).toBe(5);
+    expect(parseSchedule('@every 16m')).toBe(15);
+    expect(parseSchedule('@every 25m')).toBe(15);
+    expect(parseSchedule('@every 45m')).toBe(30);
+    expect(parseSchedule('@every 1h2m')).toBe(60);
+    expect(parseSchedule('@every 10h2m10s')).toBe(60);
   });
 
   it('api schema', async () => {