Feature request: Time based feature flags actions

[ran-isenberg]

#1554:
Feature request: Time based feature flags actions

Summary

Use cases:

• Enable maintenance mode during a weekend
• Disable support/chat feature after working hours
• Launch a new feature on a specific date and time

Changes

1. Added new condition actions, key and values (see user experience examples)
2. Added tests
3. Added documentation
4. Added new schema validation

New actions: SCHEDULE_BETWEEN_TIME_RANGE, SCHEDULE_BETWEEN_DATETIME_RANGE, SCHEDULE_BETWEEN_DAYS_OF_WEEK

New keys: CURRENT_TIME, CURRENT_DATETIME, CURRENT_DAY_OF_WEEK

New values : a dict of START and END string values and a list of all weekdays. Also an optional TIMEZONE field supporting IANA Time zones. When not specified, we assume UTC.

Assumption: time is in 24 hours format, start time is always smaller than end time and it does not overlap a day.

User experience

1. Time range

As a customer, I'd like to flip a static flag value when evaluated between 11:11 and 23:59.

{
    "my_feature": {
        "default": false,
        "rules": {
            "lambda time is between UTC 11:11-23:59": {
                "when_match": true,
                "conditions": [
                    {
                        "action": "SCHEDULE_BETWEEN_TIME_RANGE",
                        "key": "CURRENT_TIME",
                        "value": {
                            "START_TIME": "11:11",
                            "END_TIME": "23:59",
                            "TIMEZONE": "Europe/Copenahgen"
                        }
                    }
                ]
            }
        }
    }
}

2. Date range

As a customer, I'd like to flip a static flag when evaluated between full datetime ranges 2022-10-05T12:15:00Z to 2022-10-10T12:15:00Z.

{
    "my_feature": {
        "default": false,
        "rules": {
            "lambda time is between UTC october 5th 2022 12:14:32PM to october 10th 2022 12:15:00 PM": {
                "when_match": true,
                "conditions": [
                    {
                        "action": "SCHEDULE_BETWEEN_DATETIME_RANGE",
                        "key": "CURRENT_DATETIME",
                        "value": {
                            "START_TIME": "2022-10-05T12:15:00",
                            "END_TIME": "2022-10-10T12:15:00",
                            "TIMEZONE": "America/Los_Angeles"
                        }
                    }
                ]
            }
        }
    }
}

Selected days

As a customer, I'd like to flip a static flag when evaluated between Monday to Friday.

{
    "my_feature": {
        "default": false,
        "rules": {
            "match only monday through friday": {
                "when_match": true,
                "conditions": [
                    {
                        "action": "SCHEDULE_BETWEEN_DAYS_OF_WEEK",
                        "key": "CURRENT_DAY_OF_WEEK",
                        "value": {
                            "DAYS": [
                               "MONDAY",
                               "TUESDAY",
                               "WEDNESDAY",
                               "THURSDAY",
                               "FRIDAY"
                             ],
                             "TIMEZONE": "America/New_York" 
                        }
                    }
                ]
            }
        }
    }
}

Day and time range combined

As a customer, I'd like to flip a static flag when evaluated Monday-Friday between 11:00 and 23:00 (UTC).

{
    "my_feature": {
        "default": false,
        "rules": {
            "match when lambda time is between UTC 11:00-23:00 and day is either monday or thursday": {
                "when_match": true,
                "conditions": [
                    {
                        "action": "SCHEDULE_BETWEEN_TIME_RANGE",
                        "key": "CURRENT_TIME",
                        "value": {
                            "START_TIME": "11:00",
                            "END_TIME": "23:00"
                        }
                    },
                    {
                        "action": "SCHEDULE_BETWEEN_DAYS_OF_WEEK",
                        "key": "CURRENT_DAY_OF_WEEK",
                        "value": 
                            "DAYS": [
                                "MONDAY",
                                "THURSDAY"
                            ]
                    }
                ]
            }
        }
    }
}

Checklist

If your change doesn't seem to apply, please leave them unchecked.

• Meet tenets criteria
• I have performed a self-review of this change
• Changes have been tested
• Changes are documented
• PR title follows conventional commit semantics

Is this a breaking change?

RFC issue number:

Checklist:

• Migration process documented
• Implement warnings (if it can live side by side)

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

Tasks

• Update documentation with realistic use cases
• Rename _UTC suffix in keys, and implement the timezone field on the value (use python-dateutil)
• Run a performance assessment
• Review logging information
• Review docstrings and comments

[ran-isenberg]

@leandrodamascena & @heitorlessa FYI.
I implemented all the actions but there's a lot of work to be done.

1. Validation - i think we need to move to pydantic and validate ALL the configuration with it
2. Think about enums structure, maybe it's a bit confusing the way it is now, maybe split the time keys/values into even smaller enumbs.
3. mock time in tests - they fail becuase the conditions dont match
4. add tests where we mock them not to match
5. optimizations - i did a naive solution where i always calc the current time again and reparse the conditions into datetime. maybe we can use a cache for those conditions datetime parsing etc.

WDYT?
do you want to work on this together and split it?

i think we should start by merging our code and doing a quick CR

[leandrodamascena]

Suggested change

	# rule based actions have no user context. the context is the condition key
	if cond_action == schema.RuleAction.TIME_RANGE.value or schema.RuleAction.TIME_SELECTED_DAYS:
	context_value = condition.get(schema.CONDITION_KEY)
	# rule based actions have no user context. the context is the condition key
	if cond_action == schema.RuleAction.TIME_RANGE.value or schema.RuleAction.TIME_SELECTED_DAYS:
	context_value = condition.get(schema.CONDITION_KEY)
	if cond_action == schema.RuleAction.TIME_SELECTED_DAYS:
	if not isinstance(cond_value,list):
	cond_value = [cond_value]

With user experience in mind, a value for this condition can be a simple string or a list, so we convert it to a list if it's a string.
That makes sense?

[ran-isenberg]

Not sure I agree with this one. I see this action as "IN" so it should be a list.
If you open it to duality of values, it's more confusing.
It also makes inputs such as 'MONDAY, TUESDAY' break the code since you will turn it into a ['MONDAY, TUESDAY' ] which is not correct. I think having it as a list, and validating it as a list will keep it simple.

[leandrodamascena]

@leandrodamascena & @heitorlessa FYI. I implemented all the actions but there's a lot of work to be done.

Hi @ran-isenberg! Sorry for the delay in responding, but the days have been busy around here.
I tested this initial code and was able to validate a rule using this, but yes, there is a lot of work we need to do to do this. I made some suggestions to make things easier.

1. Validation - i think we need to move to pydantic and validate ALL the configuration with it
2. Think about enums structure, maybe it's a bit confusing the way it is now, maybe split the time keys/values into even smaller enumbs.
3. mock time in tests - they fail becuase the conditions dont match
4. add tests where we mock them not to match
5. optimizations - i did a naive solution where i always calc the current time again and reparse the conditions into datetime. maybe we can use a cache for those conditions datetime parsing etc.

For these questions, @heitorlessa will help us work it out. I'll be on parental leave for the next few days 👶.

[ran-isenberg]

@leandrodamascena & @heitorlessa FYI. I implemented all the actions but there's a lot of work to be done.

Hi @ran-isenberg! Sorry for the delay in responding, but the days have been busy around here. I tested this initial code and was able to validate a rule using this, but yes, there is a lot of work we need to do to do this. I made some suggestions to make things easier.

1. Validation - i think we need to move to pydantic and validate ALL the configuration with it
2. Think about enums structure, maybe it's a bit confusing the way it is now, maybe split the time keys/values into even smaller enumbs.
3. mock time in tests - they fail becuase the conditions dont match
4. add tests where we mock them not to match
5. optimizations - i did a naive solution where i always calc the current time again and reparse the conditions into datetime. maybe we can use a cache for those conditions datetime parsing etc.

For these questions, @heitorlessa will help us work it out. I'll be on parental leave for the next few days 👶.

I hope everything goes well @leandrodamascena !
I committed two of your suggestions and replied to the third.
I'll try to continue the work on it, it's a busy week.

I'll work on getting the time mocked properly and adding tests for fail use cases. That way when we refactor the code, it would be much easier. TDD for the win :)

[ran-isenberg]

@heitorlessa @leandrodamascena added mocked time. added test for no match. fixed bugs.
it actually works now!
docs/optimisations/schema validation are left. let me know what you think. I'm stopping here for now.

[heitorlessa]

Looking into the UX this morning as promised - One thing to speed up PR reviews later is to add some UX example(s) in the PR body.

[codecov-commenter]

Codecov Report

Base: 97.59% // Head: 97.51% // Decreases project coverage by -0.07% ⚠️

Coverage data is based on head (4b81046) compared to base (1f1dba1).
Patch coverage: 93.33% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1666      +/-   ##
===========================================
- Coverage    97.59%   97.51%   -0.08%     
===========================================
  Files          142      143       +1     
  Lines         6444     6561     +117     
  Branches       444      464      +20     
===========================================
+ Hits          6289     6398     +109     
- Misses         123      128       +5     
- Partials        32       35       +3     

Impacted Files	Coverage Δ
...ertools/utilities/feature_flags/time_conditions.py	92.59% <92.59%> (ø)
...ambda_powertools/utilities/feature_flags/schema.py	97.16% <93.25%> (-2.84%)	⬇️
...owertools/utilities/feature_flags/feature_flags.py	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[heitorlessa]

Adding this here so I can come back to it later today (and for @rubenfonseca after re:Invent)

Stream of consciousness for now:

• TIME_RANGE action is also being applied for date range intent; DATETIME_RANGE might be more accurate
• Might be a good idea to have a prefix for all scheduled-like tasks, e.g., SCHEDULE_TIME_RANGE, SCHEDULE_DAYS, SCHEDULE_DATETIME_RANGE
• Review: Revisit key values to find alternative UX candidates.
• Tests: for a separate PR maybe, we need to revisit the reading and writing tests for this utility - we can simplify a lot with builder functions.
• Malformed dates: Datetime range missing a Z or any timezone info will cause a ValueError which results in a False value during evaluation - ask @ran-isenberg what's the intended behaviour here (warn customers?)

Discuss usage of `strptime` as it's the slowest way to convert ISO string to date

We're converting ISO8601 strings to datetime objects. There are two known ways to do it:

1. datetime.strptime(value, "%Y-%m-%dT%H:%M:%S")
2. datetime.fromisoformat(value)

The first option (currently implemented) is considered the slowest for various reasons (string interpolation, string to integer, etc.).

The second option can be up to 40x faster (depending on Python version and machine; 3.9 on mine is 2x). However it doesn't parse the Z as UTC timezone like ECMA, and instead requires +00:00 format. We could easily replace Z with +00:00 - even with a string replacement is much faster than strptime.

Why worry about this?

We don't know how many combinations a single rule could have, so parsing many dates over on every execution can cause a significant slowdown in the execution. In large number of dates (thousands) the slowness can be measured in seconds.

Did a super contrived single date parsing with snakeviz to visualize it.

strptime output

fromisoformat

Quick changes pushed as I looked to understand the logic from a high level

• Performance changes (2.15s to 1.90s on functional tests)
  • Replaced dateutil TZ with datetime(..., tzinfo=timezone.utc) - reduce 3P dependency as it's available in stdlib
    • BEFORE: datetime.datetime(..., tz.gettz("UTC"))
    • AFTER: datetime.datetime(..., tzinfo=datetime.timezone.utc)
  • Coerced once
    • BEFORE: context.get(str(condition.get(schema.CONDITION_KEY)))
    • AFTER: context.get(condition.get(schema.CONDITION_KEY, ""))
  • Enum shouldn't subclass from str - it'll prevent Mypyc compilation. Looks like an old mistake (prior to this PR)
    • BEFORE: class RuleAction(str, Enum):
    • AFTER: class RuleAction(Enum):
• Fixed time/date range function comparison to use enum values over member. I highly suspect this was due to test_time_based_multiple_conditions_utc_in_between_time_range_rule_match test accidentally using an enum key over value.

---

Current schema UX at time of writing

Time range

As a customer, I'd like to flip a static flag value when evaluated between 11:11 and 23:59.

{
    "my_feature": {
        "default": false,
        "rules": {
            "lambda time is between UTC 11:11-23:59": {
                "when_match": true,
                "conditions": [
                    {
                        "action": "SCHEDULE_BETWEEN_TIME_RANGE",
                        "key": "CURRENT_HOUR_UTC",
                        "value": {
                            "START_TIME": "11:11",
                            "END_TIME": "23:59"
                        }
                    }
                ]
            }
        }
    }
}

Date range

As a customer, I'd like to flip a static flag when evaluated between 11:11 and 23:59.

{
    "my_feature": {
        "default": false,
        "rules": {
            "lambda time is between UTC october 5th 2022 12:14:32PM to october 10th 2022 12:15:00 PM": {
                "when_match": true,
                "conditions": [
                    {
                        "action": "SCHEDULE_BETWEEN_DATETIME_RANGE",
                        "key": "CURRENT_TIME_UTC",
                        "value": {
                            "START_TIME": "2022-10-05T12:15:00Z",
                            "END_TIME": "2022-10-10T12:15:00Z"
                        }
                    }
                ]
            }
        }
    }
}

Selected days

As a customer, I'd like to flip a static flag when evaluated between Monday to Friday.

{
    "my_feature": {
        "default": false,
        "rules": {
            "match only monday through friday": {
                "when_match": true,
                "conditions": [
                    {
                        "action": "SCHEDULE_BETWEEN_DAYS",
                        "key": "CURRENT_DAY_UTC",
                        "value": [
                            "MONDAY",
                            "TUESDAY",
                            "WEDNESDAY",
                            "THURSDAY",
                            "FRIDAY"
                        ]
                    }
                ]
            }
        }
    }
}

Day and time range combined

As a customer, I'd like to flip a static flag when evaluated Monday-Friday between 11:00 and 23:00.

{
    "my_feature": {
        "default": false,
        "rules": {
            "match when lambda time is between UTC 11:00-23:00 and day is either monday or thursday": {
                "when_match": true,
                "conditions": [
                    {
                        "action": "TIME_RANGE",
                        "key": "CURRENT_HOUR_UTC",
                        "value": {
                            "START_TIME": "11:00",
                            "END_TIME": "23:00"
                        }
                    },
                    {
                        "action": "TIME_SELECTED_DAYS",
                        "key": "CURRENT_DAY_UTC",
                        "value": [
                            "MONDAY",
                            "THURSDAY"
                        ]
                    }
                ]
            }
        }
    }
}

[heitorlessa]

@ran-isenberg UX wise, if I understood this right, this should support flags that should change state on time, datetime, and days - is that right?

If so, I'd keep TimeKeys as is, but change RuleAction to be more precise:

• TIME_SELECTED_DAYS -> SCHEDULE_BETWEEN_DAYS
• TIME_RANGE -> SCHEDULE_BETWEEN_TIME_RANGE
• TIME_RANGE for full dates -> SCHEDULE_BETWEEN_DATETIME_RANGE

The SCHEDULE prefix makes it easier to spot what the rule is about (time or value based). The addition of DATETIME_RANGE makes it explicit based on your logic - if I got it right - that you're comparing between the current_time, start_date, and end_date.

It'll become clearer any better variation (if any) when we write docs for it.

[ran-isenberg]

@heitorlessa Thank you for the feedback!
I've updated everything according to your suggestion and will start working on the validation.

[ran-isenberg]

@heitorlessa
"Malformed dates: Datetime range missing a Z or any timezone info will cause a ValueError which results in a False value during evaluation - ask @ran-isenberg what's the intended behaviour here (warn customers?)"

I think we need to fail with a schema validation error upon load.

[ran-isenberg]

@heitorlessa @leandrodamascena XXL now :)
finished validation so it's done now. all tests are green.
feel free to edit/change, no more tasks on my end for now.

[ran-isenberg]

@leandrodamascena will you have time this week to review and make suggestions for changes/optimizations?

[rubenfonseca]

Hi @ran-isenberg I was the one meant to look into your PR this week, but unfortunately I got sick. I'm getting better now and should start looking into your work between today and tomorrow. Looking forward to review it!

[rubenfonseca]

First time looking at this PR, with no context :) I'm very tired now, so I'll just add the main thing that worries me, and write more about it tomorrow:

I find it very arbitrary that we're creating rules for matching DAYS, and TIMES, and even entire DATERANGES. However, I left wondering, what if I also want to match certain HOURS, MINUTES or MONTHS? Will we be keep adding rule types to cover all those scenarios in the future?

OR, should we think of a better, generic way of representing (recurrent) time intervals and cover it in a just two calls (one-time time interval / recurrent time intervals)? Right now I'm re-reading ISO8601, as I feel this problem was solved before I was born :)

[ran-isenberg]

First time looking at this PR, with no context :) I'm very tired now, so I'll just add the main thing that worries me, and write more about it tomorrow:

I find it very arbitrary that we're creating rules for matching DAYS, and TIMES, and even entire DATERANGES. However, I left wondering, what if I also want to match certain HOURS, MINUTES or MONTHS? Will we be keep adding rule types to cover all those scenarios in the future?

OR, should we think of a better, generic way of representing (recurrent) time intervals and cover it in a just two calls (one-time time interval / recurrent time intervals)? Right now I'm re-reading ISO8601, as I feel this problem was solved before I was born :)

The daterange is standard ISO definition UTC time.
The days and hours:min range is a simplification for recurring events. So we are already covering hours or minutes, just not months.
In the end, it depends on user input. I find the datetime and days the most useful, at least in my company's usages but who knows. They seem pretty standard to me.

[leandrodamascena]

Hi @rubenfonseca and @ran-isenberg! One more PR to review and learn 😄

I started the review yesterday and hope to finish it on Monday! For now, I've created some scenarios to simulate a real use of this feature and so far so good.

[leandrodamascena]

Hi @rubenfonseca and @ran-isenberg!
I made some suggestions of what could be changed to improve this PR, but it's a great job indeed!

[leandrodamascena]

@rubenfonseca thanks for the fix and feedback! I have no further considerations, everything is working fine!

Hoping to hear something from @heitorlessa before merge it.

[heitorlessa]

hey! back today and got a few internal escalations to handle - ship it if @leandrodamascena and @rubenfonseca are happy

[leandrodamascena]

@rubenfonseca and @ran-isenberg! Before merging, we should rebase this branch to avoid unnecessary changes like this https://github.com/awslabs/aws-lambda-powertools-python/pull/1666/files#diff-06572a96a58dc510037d5efa622f9bec8519bc1beab13c9f251e97e657a9d4ed

[ran-isenberg]

@heitorlessa @rubenfonseca @leandrodamascena Thank you guys so much for the time and effort you put into this major feature!

[ran-isenberg]

when do you expect it to get merged and released?

[leandrodamascena]

when do you expect it to get merged and released?

Hey Ran! Yesterday and today the team was in an internal workshop and we were not able to merge this. Tomorrow we can merge that and start planning the next release.

[leandrodamascena]

This PR was closed without merging due to the fact that there were some unnecessary changes in the files and the rebase would be complicated. In consensus with @ran-isenberg, the original author of this PR, it was agreed that a new PR would be opened.

Closed in favor of #1846