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.

Sign up for GitHub

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

Jump to bottom

docs: add specification for in-process flagd provider implementations #848

Merged
merged 20 commits into from
Aug 24, 2023
Merged

docs: add specification for in-process flagd provider implementations #848

merged 20 commits into from
Aug 24, 2023

Conversation

bacherfl
Copy link
Contributor

Closes #842

This PR adds the specification for in-process flagd providers. For now I have added this to the other_ressources section of the docs, but please let me know if another directory would be more fitting for this document

@bacherfl
docs: add specification for in-process flagd provider implementations
e7d07d3 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl bacherfl requested a review from a team as a code owner August 21, 2023 10:54
@netlify
Copy link

netlify bot commented Aug 21, 2023 •
edited
Loading

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

Name Link
🔨 Latest commit 38dd3b2
🔍 Latest deploy log https://app.netlify.com/sites/polite-licorice-3db33c/deploys/64e7b8c5ac116500080807f0

@bacherfl
fix linting errors
47cdf1b 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
beeme1mr
beeme1mr reviewed Aug 21, 2023
View reviewed changes
docs/other_resources/in-process-providers/specification.md Outdated Show resolved Hide resolved
docs/other_resources/in-process-providers/specification.md Outdated Show resolved Hide resolved
docs/other_resources/in-process-providers/specification.md Outdated Show resolved Hide resolved
docs/other_resources/in-process-providers/specification.md Outdated Show resolved Hide resolved
docs/other_resources/in-process-providers/specification.md Outdated Show resolved Hide resolved
docs/other_resources/in-process-providers/specification.md Outdated Show resolved Hide resolved
docs/other_resources/in-process-providers/specification.md Outdated Show resolved Hide resolved
docs/other_resources/in-process-providers/evaluators/fractional_evaluation.md Outdated Show resolved Hide resolved
@toddbaert
Copy link
Member

Thanks @bacherfl, I'll review this first thing tomorrow!

@Kavindu-Dodan
Copy link
Contributor

Overal looks really good 🙌

One suggestion, IMO it is good to add a section explaining what in-memory flagd provider is and why it is useful.

For example - flagd by default is a remote deployment (sidecar injection) where as the in-memory provider is designed to be embedded into the application. And given the embedded nature, the evaluations do not require communication outside of the process, which is desired by some architectures.

bacherfl and others added 8 commits August 22, 2023 07:35
@bacherfl @beeme1mr
Update docs/other_resources/in-process-providers/specification.md
de147ca 
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl @beeme1mr
Update docs/other_resources/in-process-providers/specification.md
e29df8a 
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl @beeme1mr
Update docs/other_resources/in-process-providers/specification.md
2e53626 
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl @beeme1mr
Update docs/other_resources/in-process-providers/specification.md
96d541c 
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl @beeme1mr
Update docs/other_resources/in-process-providers/evaluators/fractiona…
584fab5 
…l_evaluation.md

Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl @Kavindu-Dodan
Update docs/other_resources/in-process-providers/specification.md
2dae7cb 
Co-authored-by: Kavindu Dodanduwa <Kavindu-Dodan@users.noreply.github.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl @Kavindu-Dodan
Update docs/other_resources/in-process-providers/specification.md
6520137 
Co-authored-by: Kavindu Dodanduwa <Kavindu-Dodan@users.noreply.github.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
switch to relative links
e7c32cb 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
Copy link
Contributor Author

Overal looks really good 🙌

One suggestion, IMO it is good to add a section explaining what in-memory flagd provider is and why it is useful.

For example - flagd by default is a remote deployment (sidecar injection) where as the in-memory provider is designed to be embedded into the application. And given the embedded nature, the evaluations do not require communication outside of the process, which is desired by some architectures.

good suggestion, will add that to the document :)

@bacherfl
added paragraph explaining the difference between in process provider…
dadb25d 
… and flagd

Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
fix markdown linting error
1409f9f 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
bacherfl and others added 2 commits August 22, 2023 15:56
@bacherfl @toddbaert
Update docs/other_resources/in-process-providers/evaluators/semver_ev…
e073ad7 
…aluation.md

Co-authored-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl @toddbaert
Update docs/other_resources/in-process-providers/evaluators/string_co…
ee2e37e 
…mparison_evaluation.md

Co-authored-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
This was referenced Aug 22, 2023
Merged
Closed
bacherfl and others added 2 commits August 23, 2023 11:33
@toddbaert toddbaert self-requested a review August 23, 2023 13:20
toddbaert
toddbaert approved these changes Aug 23, 2023
View reviewed changes
Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

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

Approved pending #848 (comment)

bacherfl and others added 4 commits August 24, 2023 08:47
@bacherfl @toddbaert
Update docs/other_resources/in-process-providers/evaluators/fractiona…
8fd72cc 
…l_evaluation.md

Co-authored-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl @Kavindu-Dodan
Update docs/other_resources/in-process-providers/specification.md
e317cda 
Co-authored-by: Kavindu Dodanduwa <Kavindu-Dodan@users.noreply.github.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
adapted to pr review comments
2eeb575 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@toddbaert
Merge branch 'main' into doc/in-process-provider-spec
38dd3b2
@toddbaert toddbaert merged commit f91751f into open-feature:main Aug 24, 2023
7 checks passed
@Kavindu-Dodan Kavindu-Dodan mentioned this pull request Aug 25, 2023
Merged
@craigpastro craigpastro mentioned this pull request Aug 28, 2023
Merged
toddbaert added a commit that referenced this pull request Aug 28, 2023
@toddbaert
feat: support nested props in fractional evaluator (#869)
50ff739 
<!-- Please use this template for your pull request. -->
<!-- Please use the sections that you need and delete other sections -->

## This PR
<!-- add the description of the PR here -->

This PR adds the ability to use nested properties for the bucket key in
fractional evaluation. It is a breaking change from the current
behaviour. If the bucket key is not provided it will default to `{"cat":
[{"var":"$flagd.flagKey"}, {"var":"targetingKey"}]}`.

@toddbaert I am not sure that my approach to the missing bucket key is
the best. I would have preferred to add `{"cat":
[{"var":"$flagd.flagKey"}, {"var":"targetingKey"}]}` to the targeting
object and let JsonLogic handle it, but it would be a bit clumsy and
involve string manipulation. I don't think there is a nice way to do it,
but I'll play around a bit more.

### Related Issues
<!-- add here the GitHub issue that this PR resolves if applicable -->

See #848.
Closes #843.

### Notes
<!-- any additional notes for this PR -->

### Follow-up Tasks
<!-- anything that is related to this PR but not done here should be
noted under this section -->
<!-- if there is a need for a new issue, please link it here -->

### How to test
<!-- if applicable, add testing instructions under this section -->

---------

Signed-off-by: Craig Pastro <pastro.craig@gmail.com>
Co-authored-by: Todd Baert <todd.baert@dynatrace.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beeme1mr beeme1mr beeme1mr approved these changes

@Kavindu-Dodan Kavindu-Dodan Kavindu-Dodan approved these changes

@toddbaert toddbaert toddbaert approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[DOC] document algorithms and in-process provider behavior
4 participants
@bacherfl @toddbaert @Kavindu-Dodan @beeme1mr