Add user-friendly cron editor to Rollup Job Wizard #23384

cjcenizal · 2018-09-21T00:29:48Z

Todo:

Fix bug where moving forward a step and then back to logistics results in the cron editor being reset
Rename "Interval" label to "Frequency"
Do test to see what happens when a cron is saved that doesn't make sense, e.g. yearly on Sept 31st. Looks like the API is rejecting date values greater than 23.

…nd restore the simple editor values.

- Move timezone up into date histogram area. - Move delay into "Schedule" section and add "1d" as the default value. - Update copy to describe how delay is used to adjust for ingest latency issues. - Remove ms and y examples, since they won't be very useful.

…ed by the API.

elasticmachine · 2018-09-21T01:27:04Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…d of seconds.

elasticmachine · 2018-09-22T01:11:15Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-09-22T01:37:33Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

jen-huang

This wizard is looking sooooo gooood!! I found the cron editor really intuitive to use. Great job ingesting all that cron syntax information and especially baking in localization from the get-go.

I have a few comments but nothing that blocks the functionality, so feel free to address them with this PR or the next pass.

LGTM!

jen-huang · 2018-09-25T00:36:30Z

x-pack/plugins/rollup/public/crud_app/sections/job_create/steps_config/index.js

      rollupPageSize: '',
+      rollupDelay: '1d',


I don't think we should have a default delay value, since delay is optional and sounds like it won't be needed for most use cases

similarly, if there is no delay value, it shouldn't be sent with the payload:

Good catch on not sending an empty default! Will fix.

I think it was @eskibars who recommended 1d as a good common default. I'll add a comment to explain this. Shane correct me if I'm wrong, but I believe you said this default would be useful for new users who aren't sure about the kind of ingest latency they'll be encountering.

I did. It is optional and while it may not be needed for many cases, I think 1d may be practical. Basically, if everything is working smoothly (e.g. nobody's restarted a server that's running a filebeat process), then data should more or less com in on time and we wouldn't need a delay. But I think in many real-world cases, servers will go down for a few hours as they're being restarted. 1d would allow them that period to come back up and the "expense" is pretty negligible in most cases: 1 day of extra non-rolled-up data.

Perfect! Thanks for the clarification.

jen-huang · 2018-09-25T00:42:17Z

...ugins/rollup/public/crud_app/sections/job_create/steps/components/cron_editor/cron_editor.js

@@ -0,0 +1,362 @@
+/*


this is such a cool component! 😍 I can totally see this potentially being reused if we have a need for cron configuration via UI elsewhere!

Thanks! Doesn't Watcher use cron as part of its config? I'm guessing it does since the cron info comes from the Watcher docs. Maybe we can reuse this within the Watcher UI.

I just saw this. Yes, Watcher does use cron. I suspect we'll have other components that will want this type of scheduled task config as well.

jen-huang · 2018-09-25T00:44:35Z

x-pack/plugins/rollup/public/crud_app/sections/job_create/steps/step_logistics.js

+      // Restore the last value of the simple cron editor.
+      rollupCron: simpleRollupCron,
+    });
+    // hideAdvancedCron();


is this line needed?

Ha no, will delete.

jen-huang · 2018-09-25T01:08:58Z

...ugins/rollup/public/crud_app/sections/job_create/steps/components/cron_editor/cron_editor.js

+import { CronMonthly } from './cron_monthly';
+import { CronYearly } from './cron_yearly';
+
+function makeSequence(min, max) {


this seems somewhat inefficient since the ranges and subsequent values created from this loop are all fixed and will not change over time. I'm not completely sure how to do away with this so I'm adding this comment to open discussion.

could the MINUTE_OPTIONS and HOUR_OPTIONS be hardcoded constants (in another file)?

could DAY_OPTIONS and MONTH_OPTIONS be retrieved from moment? then moment-local can take care of localization

DATE_OPTIONS is the toughest one. probably can't avoid a [1...31] loop here but maybe can be localized with moment as well? examples:
moment('1/3', 'M/D').format('Do'); returns 3rd
moment('1/31', 'M/D').format('Do'); returns 31st

I think you have a good point. I think I'd prefer to just hardcode everything in a constants file and avoid a dependency on moment if we can avoid it.

cjcenizal · 2018-09-25T02:46:43Z

@jen-huang I started making changes to the date stuff but it seemed to be adding complexity rather than removing it. I think it makes sense to keep it as-is because the code is clear and flexible in its current form, and I don't think it costs too much CPU to make these calculations unaffordable. That said I'm definitely still open to change it at any point down the road.

elasticmachine · 2018-09-25T02:52:04Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

* Add simple CronEditor. * Change label from Interval to Frequency. * Update cron logic to include a year field. * Fix bug in cron format, which incorrectly started with minutes instead of seconds. * Remove Data Source section. - Move timezone up into date histogram area. - Move delay into "Schedule" section and add "1d" as the default value. - Update copy to describe how delay is used to adjust for ingest latency issues. - Remove ms and y examples, since they won't be very useful.

cjcenizal added 4 commits September 20, 2018 12:18

Add basic CronEditor.

e12ad4b

Redesign CronEditor for clarity.

cc5e11a

Add toggle for switching between advanced and simple editors. Cache a…

da3a5de

…nd restore the simple editor values.

Remove Data Source section.

db5c69b

- Move timezone up into date histogram area. - Move delay into "Schedule" section and add "1d" as the default value. - Update copy to describe how delay is used to adjust for ingest latency issues. - Remove ms and y examples, since they won't be very useful.

cjcenizal added the :Management label Sep 21, 2018

cjcenizal requested review from jen-huang and maryia-lapata September 21, 2018 00:29

Fix cron expression bug by appending a question mark, which is requir…

bcddc68

…ed by the API.

cjcenizal added 6 commits September 21, 2018 09:22

Change label from Interval to Frequency.

2df89f6

Fix bug where cron editor state was being lost.

8b065f3

Update cron logic to include a year field.

55a6687

Fix bug in cron format, which incorrectly started with minutes instea…

79902fb

…d of seconds.

Add small titles to Schedule subsections.

234ca72

Fix Week frequency off-by-one error.

9557732

jen-huang approved these changes Sep 25, 2018

View reviewed changes

Address feedback.

8ad202d

cjcenizal merged commit 199c875 into elastic:feature/rollups Sep 25, 2018

cjcenizal deleted the rollups-cron-editor branch September 25, 2018 02:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add user-friendly cron editor to Rollup Job Wizard #23384

Add user-friendly cron editor to Rollup Job Wizard #23384

cjcenizal commented Sep 21, 2018 •

edited

Loading

elasticmachine commented Sep 21, 2018

elasticmachine commented Sep 22, 2018

elasticmachine commented Sep 22, 2018

jen-huang left a comment

jen-huang Sep 25, 2018

cjcenizal Sep 25, 2018

eskibars Sep 25, 2018

cjcenizal Sep 25, 2018

jen-huang Sep 25, 2018

cjcenizal Sep 25, 2018 •

edited

Loading

eskibars Sep 25, 2018

jen-huang Sep 25, 2018

cjcenizal Sep 25, 2018

jen-huang Sep 25, 2018

cjcenizal Sep 25, 2018

cjcenizal commented Sep 25, 2018

elasticmachine commented Sep 25, 2018

Add user-friendly cron editor to Rollup Job Wizard #23384

Add user-friendly cron editor to Rollup Job Wizard #23384

Conversation

cjcenizal commented Sep 21, 2018 • edited Loading

elasticmachine commented Sep 21, 2018

💔 Build Failed

elasticmachine commented Sep 22, 2018

💔 Build Failed

elasticmachine commented Sep 22, 2018

💔 Build Failed

jen-huang 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

Choose a reason for hiding this comment

cjcenizal Sep 25, 2018 • 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

cjcenizal commented Sep 25, 2018

elasticmachine commented Sep 25, 2018

💔 Build Failed

cjcenizal commented Sep 21, 2018 •

edited

Loading

cjcenizal Sep 25, 2018 •

edited

Loading