Repository navigation
Conversation
Identity overrides segments are indexed by identifier, and each identity is evaluated with its own only, narrowed to its identifier. Evaluation cost no longer grows with the number of identity overrides. Environment flags are evaluated once per environment document. Refs #198
for more information, see https://pre-commit.ci
`SegmentOverridesIndex` is now the read-only mapping consumers take, so `SegmentOverridesMapping` goes. Indexes are built as dicts of a new `SegmentContexts` alias, which also shortens repeated annotations.
One type for every index, built or read: `dict` and the per-identity `ChainMap` are both mutable mappings.
emyller
left a comment
There was a problem hiding this comment.
Looks solid in its logic. Left a few comments that aim to contribute to interface and sticking to testing patterns.
I'd also advise against using base to name anything. It does not add meaning.
| any other segment.""" | ||
| if (segment_context.get("metadata") or {}).get("source") != "identity_overrides": | ||
| return None | ||
| # As built by `map_identifiers_to_identity_overrides_rules`. |
There was a problem hiding this comment.
nit: Maybe this comment hints at a missing type? 🤔
| def map_identifiers_to_identity_overrides_rules( | ||
| identifiers: typing.List[str], | ||
| ) -> typing.List[SegmentRule]: |
There was a problem hiding this comment.
| def map_identifiers_to_identity_overrides_rules( | |
| identifiers: typing.List[str], | |
| ) -> typing.List[SegmentRule]: | |
| def map_identifiers_to_identity_overrides_rule( | |
| identifiers: typing.List[str], | |
| ) -> SegmentRule: # or `IdentityOverrideSegmentRule` (see [other thread](https://github.com/Flagsmith/flagsmith-python-client/pull/262/changes#r4325989902)) |
| # Segment contexts by their key in the evaluation context. | ||
| SegmentContexts = typing.Dict[str, SegmentContext[SegmentMetadata, FeatureMetadata]] | ||
| # Feature name -> segment contexts overriding that feature. | ||
| SegmentOverridesIndex = typing.MutableMapping[str, SegmentContexts] |
There was a problem hiding this comment.
nit
| # Segment contexts by their key in the evaluation context. | |
| SegmentContexts = typing.Dict[str, SegmentContext[SegmentMetadata, FeatureMetadata]] | |
| # Feature name -> segment contexts overriding that feature. | |
| SegmentOverridesIndex = typing.MutableMapping[str, SegmentContexts] | |
| # SegmentKey = FeatureName = str | |
| SegmentContexts = typing.Dict[SegmentKey, SegmentContext[SegmentMetadata, FeatureMetadata]] | |
| SegmentOverridesIndex = typing.MutableMapping[FeatureName, SegmentContexts] |
| # When | ||
| # the environment is refreshed | ||
| local_eval_flagsmith._evaluation_context = evaluation_context | ||
| local_eval_flagsmith.get_environment_flags() | ||
|
|
||
| # Then | ||
| assert spy.call_count == 2 |
There was a problem hiding this comment.
note: Minor pattern break makes me wonder if this portion is worth a new test case.
| assert local_evaluation.overrides_index == {} | ||
|
|
||
|
|
||
| def test_local_evaluation__get_identity_context__no_identity_overrides__returns_base( |
There was a problem hiding this comment.
| def test_local_evaluation__get_identity_context__no_identity_overrides__returns_base( | |
| def test_local_evaluation__get_identity_context_no_identity_overrides__returns_base( |
There was a problem hiding this comment.
ffd2d3e + addressed the naming concern re: base.
| assert len(identity_segments) == 1 | ||
| [segment] = identity_segments | ||
| assert segment["rules"][0]["conditions"] == [ | ||
| { | ||
| "property": "$.identity.identifier", | ||
| "operator": "IN", | ||
| "value": [identifier], | ||
| } | ||
| ] |
There was a problem hiding this comment.
| assert len(identity_segments) == 1 | |
| [segment] = identity_segments | |
| assert segment["rules"][0]["conditions"] == [ | |
| { | |
| "property": "$.identity.identifier", | |
| "operator": "IN", | |
| "value": [identifier], | |
| } | |
| ] | |
| assert identity_segments == [{...}] |
There was a problem hiding this comment.
Since 2276774 the segment key is deterministic, so the whole segment is now asserted literally.
The client kept the evaluation context next to the `LocalEvaluation` built from it. It now keeps the `LocalEvaluation` only, whose `context` is the context without identity overrides segments, formerly `base_context`. Identity overrides segments are kept apart, by key. Also drops name mangling, as `__evaluation_context` goes.
`map_identifiers_to_identity_overrides_rules` built a single rule, so it is now `map_identifiers_to_identity_overrides_rule`, returning that rule.
Identity overrides conditions are typed as `InOperatorSegmentCondition` where they are built, and identifiers are read back as such, rather than cast to a list of strings with a comment.
Replaces comments explaining what the keys are.
Also drops `base` from test and variable names.
3e37352 to
8fd7a17
Compare
Identity overrides were mapped to segments, one per unique set of overrides, merged into the evaluation context, then picked back out of it by `LocalEvaluation` and narrowed to an identity on every evaluation. They are now mapped straight from the environment document to a segment per identifier, kept out of the evaluation context. `LocalEvaluation` indexes each one's overrides once per environment document, so an identity evaluation only adds its own segment.
Drops comments, shortens the `LocalEvaluation` docstring, and specs the `LocalEvaluation` mocks in streaming manager tests.
|
Updated the logic to map env doc's |
Contributes to #198.
In this PR, we resolve two performance regressions in local evaluation mode, both dating from the move to the evaluation context engine (4.0.0/5.0.0).
Evaluation state now lives in a new
LocalEvaluationobject, rebuilt on every environment document refresh.Environment flags and single identity flags are now well ahead of 3.8.0.
An
all_flags()call for an identity is still 7–18% slower than 3.8.0; the remaining gap will be closed engine-side.Benchmark: gist