feat: add flag key to hash in fractional evaluation

[craigpastro]

This PR

This PR adds the flag key as input to the hash in fractional evaluation. Since the evaluation function only has access to user-supplied logic and context, which almost certainly will not contain the flag key, in order to get this data to the evaluation function I add a flagdProperties struct to the context via a namespaced key $flagd. The flagdProperties struct contains the flag key, which is then used as input to the hash.

Related Issues

Part 2 of #843.

Notes

Follow-up Tasks

How to test

[netlify]

✅ Deploy Preview for polite-licorice-3db33c canceled.

Name	Link
🔨 Latest commit	8c59388
🔍 Latest deploy log	https://app.netlify.com/sites/polite-licorice-3db33c/deploys/64e601bcf53da90008d653b2

[toddbaert]

@craigpastro

Could we add another test that just directly references $flagd.flag_key in a rule? Something that returns a value if flagd.flag_key === {some-known-value}. I know nobody would often write a rule like this in real life, but it will assure that we are actually populating the value as expected. See json logic do here.

[toddbaert]

Related: #851

[craigpastro]

Could we add another test that just directly references $flagd.flag_key in a rule? Something that returns a value if flagd.flag_key === {some-known-value}. I know nobody would often write a rule like this in real life, but it will assure that we are actually populating the value as expected. See json logic do here.

I've added a test. Is it what you had in mind?

[toddbaert]

Once this and this are resolved, I think I can approve.

[toddbaert]

Could we add another test that just directly references $flagd.flag_key in a rule? Something that returns a value if flagd.flag_key === {some-known-value}. I know nobody would often write a rule like this in real life, but it will assure that we are actually populating the value as expected. See json logic do here.

I've added a test. Is it what you had in mind?

yes!

[codecov]

Codecov Report

Merging #847 (8c59388) into main (88d832a) will decrease coverage by 0.14%.
The diff coverage is 71.87%.

@@            Coverage Diff             @@
##             main     #847      +/-   ##
==========================================
- Coverage   72.70%   72.56%   -0.14%     
==========================================
  Files          27       27              
  Lines        2729     2759      +30     
==========================================
+ Hits         1984     2002      +18     
- Misses        661      669       +8     
- Partials       84       88       +4     

Files Changed	Coverage Δ
core/pkg/eval/json_evaluator.go	84.58% <67.85%> (-2.92%)	⬇️
core/pkg/eval/fractional_evaluation.go	67.46% <100.00%> (+0.80%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[craigpastro]

The length of evaluateVariant is failing the linter. I don't see any nice way to restructure the function. I can make it pass by removing whitespace but that doesn't seem like a good idea, and if more properties are added to flagdProperties it will just fail again in the future. Added a // nolint:funlen is an option.

[toddbaert]

LGTM. I think we can differ documenting this feature until we decide to add other properties (such as current_timestamp).

We can tackle the changes described here in a separate PR.

[toddbaert]

The length of evaluateVariant is failing the linter. I don't see any nice way to restructure the function. I can make it pass by removing whitespace but that doesn't seem like a good idea, and if more properties are added to flagdProperties it will just fail again in the future. Added a // nolint:funlen is an option.

I've added the nolint. A lot of length in this func is coming from debug messages and helpful comments. It doesn't read as overly complex to me.

There's also a very small negative delta in codecov, but I'm fine with merging as is.