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

fix: erroneous warning about prop overwrite #924

Merged
merged 2 commits into from
Sep 19, 2023
Merged

fix: erroneous warning about prop overwrite #924

merged 2 commits into from
Sep 19, 2023

Conversation

craigpastro
Copy link
Member

This PR

When adding flagdProperties to the context in preparation for evaluation return a new context so that it doesn't affect any references to the current context.

Without this change:

$ make run
make run-flagd
cd flagd; go run main.go start -f file:../config/samples/example_flags.flagd.json

                 ______   __       ________   _______    ______      
                /_____/\ /_/\     /_______/\ /______/\  /_____/\     
                \::::_\/_\:\ \    \::: _  \ \\::::__\/__\:::_ \ \    
                 \:\/___/\\:\ \    \::(_)  \ \\:\ /____/\\:\ \ \ \   
                  \:::._\/ \:\ \____\:: __  \ \\:\\_  _\/ \:\ \ \ \  
                   \:\ \    \:\/___/\\:.\ \  \ \\:\_\ \ \  \:\/.:| | 
                    \_\/     \_____\/ \__\/\__\/ \_____\/   \____/_/ 

2023-09-18T19:16:39.581-0700    info    cmd/start.go:117        flagd version: dev (HEAD), built at: unknown    {"component": "start"}
2023-09-18T19:16:39.581-0700    info    file/filepath_sync.go:37        Starting filepath sync notifier {"component": "sync", "sync": "filepath"}
2023-09-18T19:16:39.582-0700    info    flag-evaluation/connect_service.go:203  metrics and probes listening at 8014    {"component": "service"}
2023-09-18T19:16:39.582-0700    info    file/filepath_sync.go:66        watching filepath: ../config/samples/example_flags.flagd.json       {"component": "sync", "sync": "filepath"}
2023-09-18T19:16:39.582-0700    info    flag-evaluation/connect_service.go:183  Flag Evaluation listening at [::]:8013  {"component": "service"}
2023-09-18T19:16:41.258-0700    warn    eval/json_evaluator.go:368      overwriting $flagd properties in the context    {"component": "evaluator", "evaluator": "json"}
2023-09-18T19:16:41.259-0700    warn    eval/json_evaluator.go:368      overwriting $flagd properties in the context    {"component": "evaluator", "evaluator": "json"}
2023-09-18T19:16:41.259-0700    warn    eval/legacy_fractional_evaluation.go:35 fractionalEvaluation is deprecated, please use fractional, see: https://flagd.dev/concepts/#migrating-from-legacy-fractionalevaluation
2023-09-18T19:16:41.259-0700    warn    eval/json_evaluator.go:368      overwriting $flagd properties in the context    {"component": "evaluator", "evaluator": "json"}

With this change:

$ make run
make run-flagd
cd flagd; go run main.go start -f file:../config/samples/example_flags.flagd.json

                 ______   __       ________   _______    ______      
                /_____/\ /_/\     /_______/\ /______/\  /_____/\     
                \::::_\/_\:\ \    \::: _  \ \\::::__\/__\:::_ \ \    
                 \:\/___/\\:\ \    \::(_)  \ \\:\ /____/\\:\ \ \ \   
                  \:::._\/ \:\ \____\:: __  \ \\:\\_  _\/ \:\ \ \ \  
                   \:\ \    \:\/___/\\:.\ \  \ \\:\_\ \ \  \:\/.:| | 
                    \_\/     \_____\/ \__\/\__\/ \_____\/   \____/_/ 

2023-09-18T19:20:29.011-0700    info    cmd/start.go:117        flagd version: dev (HEAD), built at: unknown    {"component": "start"}
2023-09-18T19:20:29.012-0700    info    file/filepath_sync.go:37        Starting filepath sync notifier {"component": "sync", "sync": "filepath"}
2023-09-18T19:20:29.013-0700    info    flag-evaluation/connect_service.go:203  metrics and probes listening at 8014    {"component": "service"}
2023-09-18T19:20:29.013-0700    info    flag-evaluation/connect_service.go:183  Flag Evaluation listening at [::]:8013  {"component": "service"}
2023-09-18T19:20:29.014-0700    info    file/filepath_sync.go:66        watching filepath: ../config/samples/example_flags.flagd.json       {"component": "sync", "sync": "filepath"}
2023-09-18T19:20:32.784-0700    warn    eval/legacy_fractional_evaluation.go:35 fractionalEvaluation is deprecated, please use fractional, see: https://flagd.dev/concepts/#migrating-from-legacy-fractionalevaluation

Both logs were triggered by the request in the bug report:

$ curl -X POST "http://localhost:8013/schema.v1.Service/ResolveAll" \
  -d '{"context":{}}' -H "Content-Type: application/json"

Related Issues

Fixes #923.

Notes

Follow-up Tasks

How to test

Signed-off-by: Craig Pastro <pastro.craig@gmail.com>
@craigpastro craigpastro requested a review from a team as a code owner September 19, 2023 02:31
@netlify
Copy link

netlify bot commented Sep 19, 2023

Deploy Preview for polite-licorice-3db33c canceled.

Name Link
🔨 Latest commit 79fa5dc
🔍 Latest deploy log https://app.netlify.com/sites/polite-licorice-3db33c/deploys/6509c9d988d4b50008c8eb77

@craigpastro craigpastro changed the title bug: create new context when adding flagd properties fix: create new context when adding flagd properties Sep 19, 2023
@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

Merging #924 (79fa5dc) into main (e1e7ca0) will decrease coverage by 0.09%.
Report is 1 commits behind head on main.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##             main     #924      +/-   ##
==========================================
- Coverage   72.71%   72.62%   -0.09%     
==========================================
  Files          28       28              
  Lines        2855     2857       +2     
==========================================
- Hits         2076     2075       -1     
- Misses        683      685       +2     
- Partials       96       97       +1     
Files Changed Coverage Δ
core/pkg/eval/json_evaluator.go 84.69% <80.00%> (-0.93%) ⬇️

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

Copy link
Member

@beeme1mr beeme1mr left a comment

Choose a reason for hiding this comment

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

Awesome, thanks! 😃

@toddbaert
Copy link
Member

LGTM!

@toddbaert toddbaert changed the title fix: create new context when adding flagd properties fix: erroneous warning about prop overwrite Sep 19, 2023
@toddbaert toddbaert merged commit 673b76a into open-feature:main Sep 19, 2023
14 of 15 checks passed
@github-actions github-actions bot mentioned this pull request Sep 19, 2023
@craigpastro craigpastro deleted the fix-extra-log-bug branch September 19, 2023 16:31
toddbaert pushed a commit that referenced this pull request Oct 12, 2023
🤖 I have created a release *beep* *boop*
---


<details><summary>flagd: 0.6.7</summary>

##
[0.6.7](flagd/v0.6.6...flagd/v0.6.7)
(2023-10-12)


### 🐛 Bug Fixes

* **deps:** update module github.com/open-feature/flagd/core to v0.6.6
([#916](#916))
([1f80e4d](1f80e4d))
* **deps:** update module
github.com/open-feature/go-sdk-contrib/providers/flagd to v0.1.17
([#759](#759))
([a2a2c3c](a2a2c3c))
* **deps:** update module github.com/spf13/viper to v1.17.0
([#956](#956))
([31d015d](31d015d))
* **deps:** update module go.uber.org/zap to v1.26.0
([#917](#917))
([e57e206](e57e206))


### 🧹 Chore

* docs rework ([#927](#927))
([27b3193](27b3193))


### 📚 Documentation

* fixed typos and linting issues
([#957](#957))
([0bade57](0bade57))
</details>

<details><summary>flagd-proxy: 0.2.12</summary>

##
[0.2.12](flagd-proxy/v0.2.11...flagd-proxy/v0.2.12)
(2023-10-12)


### 🐛 Bug Fixes

* **deps:** update module github.com/open-feature/flagd/core to v0.6.6
([#916](#916))
([1f80e4d](1f80e4d))
* **deps:** update module github.com/spf13/viper to v1.17.0
([#956](#956))
([31d015d](31d015d))
* **deps:** update module go.uber.org/zap to v1.26.0
([#917](#917))
([e57e206](e57e206))
</details>

<details><summary>core: 0.6.7</summary>

##
[0.6.7](core/v0.6.6...core/v0.6.7)
(2023-10-12)


### 🐛 Bug Fixes

* **deps:** update module github.com/prometheus/client_golang to v1.17.0
([#939](#939))
([9065cba](9065cba))
* **deps:** update module github.com/rs/cors to v1.10.1
([#946](#946))
([1c39862](1c39862))
* **deps:** update module go.uber.org/zap to v1.26.0
([#917](#917))
([e57e206](e57e206))
* **deps:** update module golang.org/x/mod to v0.13.0
([#952](#952))
([be61450](be61450))
* **deps:** update module golang.org/x/sync to v0.4.0
([#949](#949))
([faa24a6](faa24a6))
* **deps:** update module google.golang.org/grpc to v1.58.1
([#915](#915))
([06d95de](06d95de))
* **deps:** update module google.golang.org/grpc to v1.58.2
([#928](#928))
([90f1878](90f1878))
* **deps:** update module google.golang.org/grpc to v1.58.3
([#960](#960))
([fee1558](fee1558))
* **deps:** update opentelemetry-go monorepo
([#943](#943))
([e7cee41](e7cee41))
* erroneous warning about prop overwrite
([#924](#924))
([673b76a](673b76a))


### ✨ New Features

* add `$flagd.timestamp` to json evaluator
([#958](#958))
([a1b04e7](a1b04e7))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@github-actions github-actions bot mentioned this pull request Dec 2, 2023
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.

[BUG] Warning about overwriting $flagd property when perform bulk evaluation
3 participants