Skip to content
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

Evaluation with FlagTagsOperator to match any or all tags #408

Merged
merged 1 commit into from
Oct 13, 2020

Conversation

pacoguzman
Copy link
Contributor

@pacoguzman pacoguzman commented Oct 9, 2020

Finding flags to be evaluated using tags allow to pass ANY/ALL tag matching as operator

Description

Just support a new enum value ANY or ALL to determine which flags matches the tags the default is ANY. ALL just filter out results applying tags one after the other.

Motivation and Context

The main idea of the change is to allow to pass flagsTagsOperator on batch evaluation requests so we can handle scenarios where each tag used to describe the flags are independent so we could want to match flags which contains all the tags passed. Currently, tags are considered as any tag in the flags make the match.

We have tags determining the ownership of the flags, and some teams group flags inside other tags and we have some generic rules to naming it.

  • ownership -> owner:team-name
  • batch -> batch:batch-name

So as could be collisions on the names between teams we think that adding a match all operator for tags solve this easily to just evaluate on those flags matching all the tags provided. This is opt in as the default behaviour is the same as before.

How Has This Been Tested?

I've added a couple of tests in the codebase to describe the new behaviour

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@@ -1311,6 +1311,14 @@ definitions:
x-omitempty: true
items:
type: string
flagTagsOperator:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, this doc is generated, you can move the change to https://github.com/checkr/flagr/blob/master/swagger/index.yaml#L518, and then run make gen to get the same result

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I wasn't sure how this work


fs := cache.GetByTags(evalContext.FlagTags)
fs := cache.GetByTags(evalContext.FlagTags, operator)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for readability, I think you can just pass the evalContext.FlagTagsOperator as *string to cache.GetByTags, and then check if if operator == nil || *operator == models.EvaluationBatchRequestFlagTagsOperatorANY there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks reads better

if !ok {
// clear as no flags
results = map[uint]*entity.Flag{}
continue
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the tag is not there, I think should we just return nil here given the ALL operator.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're totally right

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But is it correct to return nil or should we return an empty list?

pkg/handler/eval_cache.go Show resolved Hide resolved
missingFIDs := []uint{}
for fID := range results {
if fSet[fID] == nil {
missingFIDs = append(missingFIDs, fID)
}
}
for _, fID := range missingFIDs {
delete(results, fID)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about one pass of the keys in results, and just delete the keys not the current fSet?

				for fID := range results {
					if _, ok := fSet[fID]; !ok {
						delete(results, fID)
					}
				}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I wasn't sure if in Go is possible to modify a slice while iterating it. Changing it for sure

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

related topic https://stackoverflow.com/questions/23229975/is-it-safe-to-remove-selected-keys-from-map-within-a-range-loop, I also learned that delete marks the keys as deleted, thus safe to keep iterating.

@pacoguzman pacoguzman force-pushed the batch-eval-tags-operator branch from af774ce to 756a071 Compare October 11, 2020 05:53
@pacoguzman
Copy link
Contributor Author

I've applied all your feedback as that makes a lot of sense and simplify the solution. Thanks for the review

@codecov-io
Copy link

codecov-io commented Oct 11, 2020

Codecov Report

Merging #408 into master will decrease coverage by 1.08%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #408      +/-   ##
==========================================
- Coverage   82.99%   81.90%   -1.09%     
==========================================
  Files          27       27              
  Lines        1752     1603     -149     
==========================================
- Hits         1454     1313     -141     
+ Misses        214      211       -3     
+ Partials       84       79       -5     
Impacted Files Coverage Δ
pkg/handler/eval_cache.go 77.04% <77.27%> (-2.96%) ⬇️
pkg/handler/eval.go 86.02% <100.00%> (-0.58%) ⬇️
pkg/entity/db.go 71.42% <0.00%> (-5.05%) ⬇️
pkg/handler/data_recorder_pubsub.go 47.05% <0.00%> (-2.95%) ⬇️
pkg/entity/variant.go 85.71% <0.00%> (-2.53%) ⬇️
pkg/handler/eval_cache_fetcher.go 81.25% <0.00%> (-2.32%) ⬇️
pkg/config/config.go 78.12% <0.00%> (-2.16%) ⬇️
pkg/handler/data_record_frame.go 86.66% <0.00%> (-1.91%) ⬇️
pkg/entity/flag.go 94.11% <0.00%> (-1.34%) ⬇️
pkg/entity/flag_snapshot.go 35.41% <0.00%> (-1.32%) ⬇️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae91b50...8377d3a. Read the comment docs.

results[fID] = f
}
} else {
if len(results) > 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one more thing to optimize is that you can return early when len(results) == 0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, after each delete loop we check if there is still any match to continue or return early.

@pacoguzman pacoguzman force-pushed the batch-eval-tags-operator branch from 756a071 to 8377d3a Compare October 13, 2020 05:01
Copy link
Collaborator

@zhouzhuojie zhouzhuojie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@zhouzhuojie zhouzhuojie merged commit 3b50881 into openflagr:master Oct 13, 2020
@pacoguzman
Copy link
Contributor Author

pacoguzman commented Oct 13, 2020 via email

@pacoguzman pacoguzman deleted the batch-eval-tags-operator branch October 15, 2020 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants