App Configuration: recurring time window filter #40093

ivywei0125 · 2024-05-09T12:54:33Z

Add "Recurrence" parameter for TimeWindowFilter using Outlook-style schema:
https://learn.microsoft.com/en-us/graph/outlook-schedule-recurring-events#using-patterns-and-ranges-to-create-recurring-events

Example:

feature-management:
  feature-flags:
    feature-v:
      enabled-for:
        -
         name: TimeWindowFilter
          parameters:
            start: "Fri, 22 Mar 2024 20:00:00 GMT",
            end: "Mon, 01 July 2019 00:00:00 GMT",
            recurrence:
              pattern:
                type: "Daily",
                interval: 1
              range:
                type: "NoEnd"

The main logic of how to check whether current time is within any recurring time window:
Find the previous occurrence of the recurring time window, let's call it prevOccurrenceStart
Check whether the current time: time is within the time window: prevOccurrenceStart ~ prevOccurrenceStart + End - Start

… to time window settings

azure-sdk · 2024-05-09T13:02:01Z

API change check

APIView has identified API level changes in this PR and created following API reviews.

com.azure.spring:spring-cloud-azure-feature-management

sdk/spring/spring-cloud-azure-feature-management/README.md

...pring/cloud/feature/management/implementation/timewindow/recurrence/RecurrenceValidator.java

sdk/spring/spring-cloud-azure-feature-management/README.md

ivywei0125 · 2024-05-30T05:18:02Z

sdk/spring/spring-cloud-azure-feature-management/README.md

+
+- Parameters
+
+ | Property | Relevance | Description |


@mrm9084 I merged all parameters to one table. It's clear that we support these 2 types for recurrence patter. But the shortcoming is that I need to declare the supported and unsupported types for each property.
Please let me know if you have any ideas about how to make it better.

ivywei0125 · 2024-05-30T05:51:47Z

...gement/src/main/java/com/azure/spring/cloud/feature/management/filters/TimeWindowFilter.java


- if (!StringUtils.hasText(start) && !StringUtils.hasText(end)) {
+ if (settings.getStart() == null && settings.getEnd() == null) {
 LOGGER.warn("The {} feature filter is not valid for feature {}. It must specify either {}, {}, or both.",


@mrm9084 Shall we throw exception here?
In the validations of recurrence, I just throw exception, because if there're invalid value for the enum type, it's easier to throw exception. Maybe we need to keep consistent?

No, I would double check with DotNet, but I think we decided most "invalid" cases result in logs and disabled features.

A similar thing happens with requesting a feature flag that doesn't exist, you just get false returned.

It seems that we may have had a design change where we now throw more errors than just fail. We need to be careful that we don't make a previous valid setup/non error setup to throw an error as that would be a breaking change.

ivywei0125 · 2024-05-30T06:02:28Z

sdk/spring/spring-cloud-azure-feature-management/README.md

+
+ | Property | Relevance | Description |
+ |-----------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+ | **type** | Required | Two valid values: `Daily`, `Weekly`. |


Actually, this type can be optional, because we have a default value as Daily.
My concern is that if I announce all parameters to be optional, then maybe users will not define any parameters in recurrence pattern like the following.

start: "Mon, 13 May 2024 02:00:00 GMT", end: "Mon, 13 May 2024 03:00:00 GMT", recurrence: range: type: "NoEnd"

Shall we support this? Or throw invalid exception since there's no pattern parameter? @mrm9084

zhiyuanliang-ms · 2024-05-31T06:38:23Z

...in/java/com/azure/spring/cloud/feature/management/implementation/models/RecurrenceRange.java

+ /**
+ * @param numberOfOccurrences the repeat times to be set
+ * */
+ public void setNumberOfOccurrences(int numberOfOccurrences) {


This is different from the setInterval(interger interval)

zhiyuanliang-ms · 2024-05-31T06:39:57Z

.../java/com/azure/spring/cloud/feature/management/implementation/models/RecurrencePattern.java

+ /**
+ * @param interval the time units to be set
+ * */
+ public void setInterval(Integer interval) {


You use int type in public void setNumberOfOccurrences(int numberOfOccurrences)

Is there any reason use Integer here? It doesn't make sense to do null check here, since Interval has default value 1.

zhiyuanliang-ms · 2024-05-31T07:46:31Z

...pring/cloud/feature/management/implementation/timewindow/recurrence/RecurrenceValidator.java

+
+ // Check whether "Start" is a valid first occurrence
+ final RecurrencePattern pattern = settings.getRecurrence().getPattern();
+ if (pattern.getDaysOfWeek().stream().noneMatch((dayOfWeekStr) ->


Should it be dayOfWeek instead of dayOfWeekStr? getDaysOfWeek returns List

sdk/spring/spring-cloud-azure-feature-management/README.md

mrm9084 · 2024-05-30T21:29:45Z

sdk/spring/spring-cloud-azure-feature-management/README.md

+ numberOfOccurrences: 3
+```
+
+The `recurrence` settings is made up of two parts: `pattern` (how often the time window will repeat) and `range` (for how long the recurrence pattern will repeat). To create a recurrence rule, you must specify both `pattern` and `range`. Any pattern type can work with any range type.


It should be 3/4 parts as the start/end are required and give the period of time that the flag is enabled for.

Also, we should mention start/end here.

I use "2 parts" because there's only 2 parameters behind the "recurrence" parameter.
I agree that we should mention start/end here.

Updated to the following description.

To create a recurrence rule, you need to specify 3 parts: `start`, `end` and `recurrence`. The `start` and `end` parameters define the time window which need to recur periodically. The `recurrence` parameter is made up of two parts: `pattern` (how often the time window will repeat) and `range` (for how long the recurrence pattern will repeat). To create a recurrence rule, you must specify both `pattern` and `range`. Any pattern type can work with any range type.

mrm9084 · 2024-05-30T21:35:29Z

sdk/spring/spring-cloud-azure-feature-management/README.md

+
+#### Recurrence Pattern
+
+There are two possible recurrence pattern types: `Daily` and `Weekly`. For example, a time window could repeat "every day", "every 3 days", "every Monday" or "every other Friday".


I don't think this does a good job explaining Daily/Weekly. It also doesn't help that a single period may not be over 24 hours but can take place over 2 days.

Daily is the Days that have passed, which isn't actually Daily. Same with Weekly. Either that or I misunderstand how this works, which is also an issue.

You should separate the Daily and Weekly examples from each other so you know which one a person may want.

Updated to the following

There are two possible recurrence pattern types: `Daily` and `Weekly`. `Daily` causes an event to repeat based on a number of days between each occurrence. For example, "every day" or "every 3 days". `Weekly` causes an event to repeat on the same day or days of the week, based on the number of weeks between each set of occurrences. For example, "every Monday" or "every other Friday".

sdk/spring/spring-cloud-azure-feature-management/README.md

mrm9084 · 2024-06-03T21:30:48Z

sdk/spring/spring-cloud-azure-feature-management/README.md

+ interval: 2,
+ daysOfWeek: 
+ - Monday
+ - Tuesday


Have you tried using this yaml. I get a json parsing error when trying to use it.

Sorry, updated

mrm9084 · 2024-06-03T21:38:13Z

...rc/main/java/com/azure/spring/cloud/feature/management/implementation/models/Recurrence.java

+/**
+ * A recurrence definition describing how time window recurs
+ * */
+public class Recurrence {


We should add the Annotation to all of the models to ignore unknown fields. It will save us at some point if a field is added to the portal by default. Wouldn't be the first time.

mrm9084 · 2024-06-03T21:44:30Z

...in/java/com/azure/spring/cloud/feature/management/implementation/models/RecurrenceRange.java

+ /**
+ * The date to stop applying the recurrence pattern
+ * */
+ private ZonedDateTime endDate = Instant.ofEpochMilli(Integer.MAX_VALUE).atZone(ZoneOffset.UTC);


What is the intent of this? Is it just an unrealistic end date to cause errors? Also, why not just 0 instead of max int? The number of milliseconds of a max int is just a few days. So instead of 01-01-1970 it's 01-25-1970?

Removed this default value.

mrm9084 · 2024-06-03T21:45:53Z

sdk/spring/spring-cloud-azure-feature-management/README.md

+ Property | Relevance | Description |
+ -----------|---------------|-------------
+ **type** | Required | Three valid values: `NoEnd`, `EndDate` and `Numbered`.
+ **endDate** | Optional | Specifies the date time to stop applying the pattern. Note that as long as the start time of the last occurrence falls before the end date, the end time of that occurrence is allowed to extend beyond it. <br /> It's required for `EndDate` type, not applicable for `NoEnd` and `Numbered` type.


endDate currently has a default value. It's not "required", The end date just has to be before 1970-01-25T20:31:23.647Z

I removed the default value of endDate. It sounds not reasonable that we have some default value.

mrm9084 · 2024-06-03T21:47:44Z

...in/java/com/azure/spring/cloud/feature/management/implementation/models/RecurrenceRange.java

+ /**
+ * The number of times to repeat the time window
+ */
+ private int numberOfOccurrences = Integer.MAX_VALUE;


This shouldn't have a default value if it's a required field. Right now the default value makes it effectively a no end filter.

Yes. I agree that no need to set a default value.

Co-authored-by: Matthew Metcalf <[email protected]>

…/com/azure/spring/cloud/feature/management/filters/TimeWindowFilter.java Co-authored-by: Matthew Metcalf <[email protected]>

…m/ivywei0125/azure-sdk-for-java into yuwe/recurring-time-window-filter

mrm9084 · 2024-06-05T21:46:44Z

sdk/spring/spring-cloud-azure-feature-management/README.md

+
+ **Note:** `start` must be a valid first occurrence which fits the recurrence pattern. For example, if we define to repeat on every other Monday and Tuesday, then the start time should be in Monday or Tuesday. <br /> Additionally, the duration of the time window cannot be longer than how frequently it occurs. For example, it is invalid to have a 25-hour time window recur every day.
+
+#### Recurrence Range


Have you tested this? I have no exception is thrown for me when I make one with it.

mrm9084 · 2024-06-05T21:48:17Z

...gement/src/main/java/com/azure/spring/cloud/feature/management/filters/TimeWindowFilter.java


- if (!StringUtils.hasText(start) && !StringUtils.hasText(end)) {
+ if (settings.getStart() == null && settings.getEnd() == null) {
 LOGGER.warn("The {} feature filter is not valid for feature {}. It must specify either {}, {}, or both.",


It seems that we may have had a design change where we now throw more errors than just fail. We need to be careful that we don't make a previous valid setup/non error setup to throw an error as that would be a breaking change.

…ead of catching the exception

ivywei0125 added 13 commits April 27, 2024 11:06

add base class: recurrence range, recurrence pattern

bd2b15a

add object mapper in time window filter for parsing the config string…

56832a9

… to time window settings

implement the logic about recurrence evaluator

18551d6

add validations

21519ab

add test case for validator

bfb0d93

add test case for evaluator

23f13f7

add more test case

20825d7

add more test case

c4f14c6

add calculateClosestStart method to calculate the closest occurrence

122541e

rename function

15e7fa2

add cache logic

c6fa94f

update README.md

449d482

update cache service

6613249

ivywei0125 self-assigned this May 9, 2024

ivywei0125 requested review from mrm9084, chenrujun, Netyyyy, saragluna and moarychan as code owners May 9, 2024 12:54

ivywei0125 changed the title ~~Yuwe/recurring time window filter~~ App Configuration: recurring time window filter May 9, 2024

github-actions bot added the azure-spring All azure-spring related issues label May 9, 2024

ivywei0125 added 4 commits May 9, 2024 21:19

ci

3365d34

ci

ac68532

move to parent path to make the package name shorter

c683ee8

update copy right

6bbbaaa

mrm9084 requested changes May 10, 2024

View reviewed changes

address comments: update README.md

218cfa9

mrm9084 requested changes May 13, 2024

View reviewed changes

sdk/spring/spring-cloud-azure-feature-management/README.md Outdated Show resolved Hide resolved

sdk/spring/spring-cloud-azure-feature-management/README.md Outdated Show resolved Hide resolved

zhiyuanliang-ms mentioned this pull request May 14, 2024

Update README to emphasize built-in filters are add automatically and adjust wording microsoft/FeatureManagement-Dotnet#448

Merged

ivywei0125 added 5 commits May 30, 2024 10:14

address comment: removed unused method

d6d815c

address comment: update symbols

2b928c9

address comment: adjust spaces

59128ac

address comment: add more test case

6ebe56c

address comment: update the README.md

dc659ad

ivywei0125 commented May 30, 2024

View reviewed changes

address comment: code style

631b357

ivywei0125 commented May 30, 2024

View reviewed changes

ivywei0125 added 4 commits May 30, 2024 14:49

address comment: typo error fix

80c6011

address comment: typo error fix

81da607

address comment: update README.md

e1c6007

address comment: update README.md

f2ca9c7

zhiyuanliang-ms reviewed May 31, 2024

View reviewed changes

mrm9084 requested changes Jun 3, 2024

View reviewed changes

ivywei0125 and others added 10 commits June 5, 2024 09:41

Update sdk/spring/spring-cloud-azure-feature-management/README.md

22059be

Co-authored-by: Matthew Metcalf <[email protected]>

Update sdk/spring/spring-cloud-azure-feature-management/README.md

9e7f6a9

Co-authored-by: Matthew Metcalf <[email protected]>

address comment: fix error in yaml example

59067dc

address comment: update README.md

155074d

Update sdk/spring/spring-cloud-azure-feature-management/src/main/java…

b59ae50

…/com/azure/spring/cloud/feature/management/filters/TimeWindowFilter.java Co-authored-by: Matthew Metcalf <[email protected]>

address comment: add @throws to java doc

b68eb82

address comment: add "ignoreUnknown" annotation

d957a1f

address comment: add year info to make it clearer

bad1e78

Merge branch 'yuwe/recurring-time-window-filter' of https://github.co…

cc86041

…m/ivywei0125/azure-sdk-for-java into yuwe/recurring-time-window-filter

address comment: add test case for time with RFC format

31c3bf7

mrm9084 requested changes Jun 5, 2024

View reviewed changes

ivywei0125 added 2 commits June 6, 2024 09:48

address comment: fix build error

a7c0271

address comment: update the test case. Should use assertThrows inst…

f04780b

…ead of catching the exception

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

App Configuration: recurring time window filter #40093

App Configuration: recurring time window filter #40093

ivywei0125 commented May 9, 2024

azure-sdk commented May 9, 2024

ivywei0125 May 30, 2024

ivywei0125 May 30, 2024

mrm9084 May 31, 2024

mrm9084 Jun 5, 2024

ivywei0125 May 30, 2024

zhiyuanliang-ms May 31, 2024

zhiyuanliang-ms May 31, 2024

zhiyuanliang-ms May 31, 2024 •

edited

mrm9084 May 30, 2024

mrm9084 May 30, 2024

ivywei0125 Jun 5, 2024

mrm9084 May 30, 2024

ivywei0125 Jun 5, 2024

mrm9084 Jun 3, 2024

ivywei0125 Jun 5, 2024

mrm9084 Jun 3, 2024

ivywei0125 Jun 5, 2024

mrm9084 Jun 3, 2024

ivywei0125 Jun 5, 2024

mrm9084 Jun 3, 2024

ivywei0125 Jun 5, 2024

mrm9084 Jun 3, 2024

ivywei0125 Jun 5, 2024

mrm9084 Jun 5, 2024

mrm9084 Jun 5, 2024


		#### Recurrence Pattern

		There are two possible recurrence pattern types: `Daily` and `Weekly`. For example, a time window could repeat "every day", "every 3 days", "every Monday" or "every other Friday".


		Note: `start` must be a valid first occurrence which fits the recurrence pattern. For example, if we define to repeat on every other Monday and Tuesday, then the start time should be in Monday or Tuesday. <br /> Additionally, the duration of the time window cannot be longer than how frequently it occurs. For example, it is invalid to have a 25-hour time window recur every day.

		#### Recurrence Range

App Configuration: recurring time window filter #40093

Are you sure you want to change the base?

App Configuration: recurring time window filter #40093

Conversation

ivywei0125 commented May 9, 2024

azure-sdk commented May 9, 2024

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

zhiyuanliang-ms May 31, 2024 • edited

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

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

zhiyuanliang-ms May 31, 2024 •

edited