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

Add getting features from database example #389

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

fffffatah
Copy link

Simple example for getting features from a database. Example includes,

  • A custom FeatureDefinitionProvider.
  • Getting features using EFCore and SQLite.

@fffffatah
Copy link
Author

@microsoft-github-policy-service agree

@zhiyuanliang-ms
Copy link
Contributor

/AzurePipeline run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

builder.Services.AddFeatureService();

/* Add feature management */
builder.Services.AddScoped<IFeatureDefinitionProvider, CustomFeatureDefinitionProvider>().AddScopedFeatureManagement();
Copy link
Contributor

@zhiyuanliang-ms zhiyuanliang-ms Mar 10, 2024

Choose a reason for hiding this comment

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

Hi, @fffffatah Maybe this is a stupid question. Would you mind explaining why CustomFeatureDefinitionProvider is registered as scoped here?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @zhiyuanliang-ms 👋, CustomFeatureDefinitionProvider is scoped here because the life-time of DbContext and the service which is used to read the features from DB are scoped. If I'm not wrong, setting the life time of DbContext as singleton might lead to issues when many concurrent calls are made. Although, DbContext can be registered as singleton and the Feature set can be marked AsNoTracking while querying, but it felt a bit risky.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you!

@zhiyuanliang-ms
Copy link
Contributor

zhiyuanliang-ms commented Mar 10, 2024

@fffffatah When I tried to run this application, it gave the result of "localhost page can not be found". This is quite misleading, which will make user doubt whether there is something wrong. So my suggestion is to add a home page and make this web app more straight forward. Let the user see what you want them to see at the home page.

Then, I tried to access localhost:12345/weatherforecast. It gave me the following error:
Exception: You need to call SQLitePCL.raw.SetProvider(). If you are using a bundle package, this is done by calling SQLitePCL.Batteries.Init().

And I tried to remove the FeatureGate above the get method of controller, it works. I guess the previous exception is because there is no database.

Would you mind including a README? (something like: example 1 example 2) The README should introduce this example application and teach users how to run it.

@fffffatah
Copy link
Author

@zhiyuanliang-ms I simplified the example for initial run and added Swagger UI for ease of use. Also, added a README to get started. Sorry about the exception, it happened due to a package mismatch.

@zhiyuanliang-ms
Copy link
Contributor

/AzurePipeline run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@zhiyuanliang-ms
Copy link
Contributor

zhiyuanliang-ms commented Mar 11, 2024

@fffffatah The application works for me now. But It seems like the CI pipeline failed. I will take some time to look into it.

Thank you for your interest and support for this repo. Appreciate you spending the time on this PR.
I want to point out the significant value of this PR to us, even if it ultimately might not be merged.

This PR fills a missing piece of examples in this repo. We provides the IFeatureDefinitionProvider interface and this interface provides the extensibility to allow users to implement their own feature definition provider. But we don't have any detailed tutorial or example about how to implement IFeatureDefinitionProvider, which was called out 3 years ago. (Previous issue can be found in #93)

This PR also lets us know how actually user will use our FM lib to work with feature flags from database. The use case of importing feature flag from database is common especially when users are migrating from other third-party feature management libs. This is super valuable for us. Really appreciate that. So even if this PR cannot be merged, it brings a good chance for us to seriously consider how to build an example app of implementing a custom feature defintion provider from a database.

Another interesting thing is that this PR uses the AddScopedFeatureManagement API which we presented in 3.1.0. The very initial idea of this API is to support Blazor server application to use Feature Management. But it turns out this API can be helpful when working with database according to your comment: #389 (comment) I will spend some time to study why database usage requires scoped services.

Thanks again for your effort. @fffffatah This PR is super valuable for us.

@zhiyuanliang-ms zhiyuanliang-ms self-assigned this Mar 11, 2024
@zhiyuanliang-ms
Copy link
Contributor

zhiyuanliang-ms commented Mar 12, 2024

@fffffatch
The Azure pipeline failed and it reported the following error. It seems the dll of sqlite is not compliant.
image

But I saw your first commit can pass the pipeline and you also used sqlite there.

@fffffatah
Copy link
Author

@fffffatch The Azure pipeline failed and it reported the following error. It seems the dll of sqlite is not compliant. image

But I saw your first commit can pass the pipeline and you also used sqlite there.

@zhiyuanliang-ms Looks like BinSkim is having trouble with native SQLite binary as it doesn't have a pdb file alongside it. Initially, the build was successful because the package was wrong and I fixed it later. Would it be okay if I get rid of SQLite all together and use SQL Server for this example? I can also update the README.md accordingly. (The only drawback here would be that users would have to have SQL Server up and running on their local machine before running the project.)

@zhiyuanliang-ms
Copy link
Contributor

@fffffatah

I guess SQL Server will not pass the Azure pipeline either. I do remember I met some trouble while including SQL stuff in some example projects.
I tried to include login/register function in an example project, which used SQL server to store user info. That PR failed to pass the Azure pipeline. But I am not 100% sure about whether using SQL server will make the PR fail to pass the CI check.

Even if a PR can not pass the pipeline check, it still can be merged. So, don't be too worried about it.

Comparing to SQL server, I think SQLite may be a better choice, because it is lighter and user can use it directly as long as the nuget package is installed.

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.

None yet

2 participants