-
Notifications
You must be signed in to change notification settings - Fork 119
Support recurring time window filter for TimeWindowFilter #256
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
Changes from all commits
cbe6156
ed31543
99b25a8
7978913
555c2da
ba806da
682ee1a
6289c3a
533e9db
96fa59b
9acdfcd
d522c85
a3a1a0c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,121 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT license. | ||
| // | ||
| using System; | ||
|
|
||
| namespace Microsoft.FeatureManagement.FeatureFilters.Cron | ||
| { | ||
| /// <summary> | ||
| /// The Cron expression with Minute, Hour, Day of month, Month, Day of week fields. | ||
| /// </summary> | ||
| internal class CronExpression | ||
| { | ||
| private static readonly int NumberOfFields = 5; | ||
|
|
||
| private readonly CronField _minute; | ||
| private readonly CronField _hour; | ||
| private readonly CronField _dayOfMonth; | ||
| private readonly CronField _month; | ||
| private readonly CronField _dayOfWeek; | ||
|
|
||
| private CronExpression(CronField minute, CronField hour, CronField dayOfMonth, CronField month, CronField dayOfWeek) | ||
| { | ||
| _minute = minute; | ||
| _hour = hour; | ||
| _dayOfMonth = dayOfMonth; | ||
| _month = month; | ||
| _dayOfWeek = dayOfWeek; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Check whether the given expression can be parsed by a CronExpression. | ||
| /// If the expression is invalid, an ArgumentException will be thrown. | ||
| /// </summary> | ||
| /// <param name="expression">The expression to parse.</param> | ||
| /// <returns>A parsed CronExpression.</returns> | ||
| public static CronExpression Parse(string expression) | ||
| { | ||
| if (expression == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(expression)); | ||
| } | ||
|
|
||
| string InvalidCronExpressionErrorMessage = $"The provided Cron expression: '{expression}' is invalid."; | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd like if we can avoid defining white space characters and instead use IsWhiteSpace I suggest
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good! |
||
| var fields = new string[NumberOfFields]; | ||
|
|
||
| int i = 0, pos = 0; | ||
|
|
||
| while (i < expression.Length) | ||
| { | ||
| if (char.IsWhiteSpace(expression[i])) | ||
| { | ||
| i++; | ||
|
|
||
| continue; | ||
| } | ||
|
|
||
| if (pos >= fields.Length) | ||
| { | ||
| throw new ArgumentException(InvalidCronExpressionErrorMessage, nameof(expression)); | ||
| } | ||
|
|
||
| int start = i; // Start of a field | ||
|
|
||
| while (i < expression.Length && !char.IsWhiteSpace(expression[i])) | ||
| { | ||
| i++; | ||
| } | ||
|
|
||
| fields[pos] = expression.Substring(start, i - start); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even better if we can use |
||
|
|
||
| pos++; | ||
| } | ||
|
|
||
| if (CronField.TryParse(CronFieldKind.Minute, fields[0], out CronField minute) && | ||
| CronField.TryParse(CronFieldKind.Hour, fields[1], out CronField hour) && | ||
| CronField.TryParse(CronFieldKind.DayOfMonth, fields[2], out CronField dayOfMonth) && | ||
| CronField.TryParse(CronFieldKind.Month, fields[3], out CronField month) && | ||
| CronField.TryParse(CronFieldKind.DayOfWeek, fields[4], out CronField dayOfWeek)) | ||
| { | ||
| return new CronExpression(minute, hour, dayOfMonth, month, dayOfWeek); | ||
| } | ||
| else | ||
| { | ||
| throw new ArgumentException(InvalidCronExpressionErrorMessage, nameof(expression)); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Checks whether the Cron expression is satisfied by the given timestamp. | ||
| /// </summary> | ||
| /// <param name="time">The timestamp to check.</param> | ||
| /// <returns>True if the Cron expression is satisfied by the give timestamp, otherwise false.</returns> | ||
| public bool IsSatisfiedBy(DateTimeOffset time) | ||
| { | ||
| /* | ||
| The current time is said to be satisfied by the Cron expression when the 'minute', 'hour', and 'month of the year' fields match the current time, | ||
| and at least one of the two 'day' fields ('day of month', or 'day of week') match the current time. | ||
| If both 'day' fields are restricted (i.e., do not contain the "*" character), the current time will be considered as satisfied when it match either 'day' field and other fields. | ||
| If exactly one of 'day' fields are restricted, the current time will be considered as satisfied when it match both 'day' fields and other fields. | ||
| */ | ||
| bool isDayMatched; | ||
|
|
||
| if (!_dayOfMonth.MatchesAll && !_dayOfWeek.MatchesAll) | ||
| { | ||
| isDayMatched = (_dayOfMonth.Match((int)time.Day) || _dayOfWeek.Match((int)time.DayOfWeek)) && | ||
| _month.Match((int)time.Month); | ||
| } | ||
| else | ||
| { | ||
| isDayMatched = _dayOfMonth.Match((int)time.Day) && | ||
| _dayOfWeek.Match((int)time.DayOfWeek) && | ||
| _month.Match((int)time.Month); | ||
| } | ||
|
|
||
| return isDayMatched && | ||
| _hour.Match((int)time.Hour) && | ||
| _minute.Match((int)time.Minute); | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really have to support an array of crontab expressions, or is one sufficient for most scenarios? Even if someone really needs more than one, they can always add more Time Window filters with different configurations.
I'm concerned this complicates the usage of the recurring Time Window filter. I can already see this complicates the UI design.
BTW, having a 'Filters' parameter in a feature filter is also confusing.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The scenario when the user want to activate a feature flag during 18:00-20:00 on every weekend and during 18:00-22:00 on holidays requires a list to specify recurring time windows with different durations.
Using multiple timewindow filters may not be a good choice, since the feature flag may also use other filters(e.g. targeting), in this case, the "RequirementType" is "All".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that the parameter name 'Filters' is kind of confusing. But the cron expression does serve as "filters" since we also have the parameter "Start" and "End" which should be considered as the timestamp when the filter starts to work("FilterStart") and when the filter ends working("FilterEnd").
The reason why we use "Filters" instead of "Recurrence" is that we will treat an empty list of "Filters" as not specified.
When "Filters" is not specifed, the TimeWindowFilter will revert to the original fixed time window filter.
The name "Filters" makes more sense.
Let's take a look at two choices:
Choice A:
"Start" : "2023-9-19-00:00",
"End": "2024-9-19-00:00",
"Filters": [ ]
Choice B:
"Start" : "2023-9-19-00:00",
"End": "2024-9-19-00:00",
"Recurrence": [ ]
If "Filters" or "Recurrence" is not specified, there is no confusion.
If "Filters" or "Recurrence" is not empty, the filter will only enable the feature during the recurring time window listed in the list "Filters" or "Recurrence". However, for the “Recurrence" choice, the parameter "Start" and "End" serve as the "RecurrenceStart" and "RecurrentEnd" instead of "FilterStart" and "FilterEnd". This will make the scenario that the "Recurrence" is specified as an empty list confusing. The function of "Start" and "End" will be inconsistent if we use name "Recurrence".
The user who uses FeatureManagement-Dotnet is expected to read the README before using it. The user who uses FeatureManagement through portal will never know the backend data schema for TimeWindowFilter unless they use "Advanced Edit"(in this case, the user should be considered as a pro who has read our doc). On the Portal, the scenarios of setting a fixed time window and setting recurring time window will have different UI and will not mention the "Filters" parameter.
For me, I will vote for the name "Filters" because it makes the role of "Start" and "End" parametes consistent. I also believe it will not cause confusion for the user who uses FeatureManagement-Dotnet lib directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure we can come up with even more complex scenarios. The question is how realistic they are. Outlook doesn't support infinite recurrence patterns, just one pattern at a time. Without the array, the parameters can be simplified to "Start", "End" and "RecurrencePattern". The UI design doesn't need an add button for more than one recurrence pattern anymore.
We know the "RequirementType" doesn't support all possible logical operations. It was a conscious decision. It's a balance between complexity and the most common scenarios. The choice of that design shouldn't be the reason for us to complicate the design of each feature filter. We have a reasonable workaround. We know there are scenarios that won't be supported out-of-box no matter how. That's where customers should consider custom filters.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do not want to use a single parameter "RecurrencePattern", then we cannot use cron expression any more. The reason is that we have to use at least three cron expressions to describe the time span 18:30`20:30. e.g. "30-59 18 * * ", " 19 * * *“ and "0-29 20 * * *".
I have proposed a lot of schema designs to describe the recurrence. Some of them are deprecated early and are not included in the design doc.
The best way to describe recurrence is to specify the duration and recurrence pattern (i.e. when we want to make the recuurence happen).
At first, I want to use two parameters "Duration" and "RecurWhen".
This represents 18:00-22:00 every Friday.
This design will use cron expression in classic way where the cron expression represents the schedule (time points instead of time spans).
But Jimmy thought we can include the duration information in the cron expression, in this way the json schema will be very simple. The above configuration can be simplified to a single cron "* 18-22 * * 5".
Jimmy thought the current design achieved the balance between simplicity, offering to customers and implementation/maintenance. I completly agree with Jimmy, that's the reason why we finally decided to use the current cron design for the recurrence configuration.
Besides, the current design also provides a benefit that it will make the process to check whether the current time is within any recurring time window very simple. We only need to check whether five fields are compliant with current timestamp.
If we use the "Duration" and "RecurWhen" design, we need to compute the previous occurrence of the cron schedule which is the start of the recurring time window and then check whether the current time is within that recurring time windows.
These are some basic scenarios we want to support(you can find it in the design doc):
a) Recur during a certain time window every day
b) Recur during a certain time window on certain days of a week
c) Recur during a certain time window on certain days of a month
I think all of them are reasonable scenarios and we should support all of them.
The current design is the simplest one which can support all of these scenarios.
In fact, the cron expression is the most compact format to describe the recurrence and it is a well-known and standardized format. We can invent some formats to describe the recurrence, but it would be better to use something standardized. (This discussion can be found in the comments of the design doc.)
If you go to see the AKS doc, you will find AKS has a feature called maintenance schedule: https://learn.microsoft.com/en-us/azure/aks/planned-maintenance.


This is a very similar scenario where user can configure some recurring time windows to let AKS cluster to upgrade.
The AKS team designs a quite complicated schema
The AKS team invents a very complex schema to descibe the recurring maintenance time window.
It will look like this.
AKS even does not support configuring this on the Portal. It can only be configured through CLI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow what you said here. I did NOT suggest we abandon the usage of cron expression. I am only suggesting we support one cron expression per Time Window filter instead of an array of them. Here is an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if I want the recurrence happen during 18:30`20:30 every day?
You cannot represent 18:30-20:30 in a single cron.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @zhenlan, I thinks scenario like
18:30-20:30 everydayis a very common scenario, not something that rarely happens. And current design does not allow us to use one single cron to describe it. Since they are closely tied together, adding two cron expressions makes more sense to me than adding two TimeWindow filters. If we don't support cron array, probablly customers will have to add more Feature filters, which means inceasing size/complexity in another level.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@juniwang Yes, we chatted offline, and I understand this now. This limitation imposes challenges on our portal to properly break down
18:30-20:30into 3 crontabs and reconstruct it back from crontabs to a human-readable time duration. We need to rethink our design choices.