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

feat: add feature flag endpoint #531

Merged
merged 14 commits into from
May 1, 2024
Merged

Conversation

daniel-codecov
Copy link
Contributor

@daniel-codecov daniel-codecov commented Apr 30, 2024

Purpose/Motivation

What is the feature? Why is this being done?

Dependent on codecov/shared#205.

Adds an endpoint at /internal/features which will accept a request body of

{
    "feature_flags": [
        "my_feature_1",
        "my_feature_2",
        "my_feature_3,
        "my_feature_4"
    ],
    "identifier_data": {
        "email": "daniel.yu@sentry.io",
        "user_id": 123,
        "org_id": 1,
        "repo_id": 123213
    }
}

and spit out a response with all the flag evaluations for that given user based on their identifier_data:

{
    "my_feature_1": 4,
    "my_feature_2": false,
    "my_feature_3": true,
    "my_feature_4": false
}

Mostly copies over the core logic for https://github.com/codecov/shared/blob/f7768f29cb0084947d4eb0c42db6c747ad3ee3f0/shared/rollouts/__init__.py#L27 except with its own caching strategy using Redis. The caching strategy here:

  1. Attempt to fetch all flags being requested from redis cache
  2. Of those flags with a cache-hit, evaluate them (figure out what variant they get)
  3. Of the flags with a cache-miss, bulk fetch them all from the db. Evaluate all these flags, and also store them in Redis which has 5 min TTL.
  4. Its actually necessary to not refresh the TTL of cache-hit keys since if they did refresh, the flags values would be forever stale if the flag is updated in django admin

Also adds tests for the endpoint

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

Copy link

codecov bot commented Apr 30, 2024

Codecov Report

Attention: Patch coverage is 95.29412% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 95.79%. Comparing base (8601246) to head (f9f3274).

✅ All tests successful. No failed tests found.

Files Patch % Lines
api/internal/feature/views.py 96.49% 2 Missing ⚠️
api/internal/feature/helpers.py 92.85% 1 Missing ⚠️
codecov/admin.py 75.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main    #531     +/-   ##
=======================================
- Coverage   95.80   95.79   -0.01     
=======================================
  Files        774     777      +3     
  Lines      17124   17208     +84     
=======================================
+ Hits       16404   16484     +80     
- Misses       720     724      +4     
Flag Coverage Δ
unit 91.51% <95.29%> (+0.01%) ⬆️
unit-latest-uploader 91.51% <95.29%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@daniel-codecov daniel-codecov marked this pull request as ready for review April 30, 2024 19:19
@daniel-codecov daniel-codecov changed the title add feature endpoint feat: add feature flag endpoint Apr 30, 2024
Copy link
Contributor

@matt-codecov matt-codecov left a comment

Choose a reason for hiding this comment

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

rather than storing the FeatureFlag in a redis cache, could you go a layer higher and store the list of flag results in the cache based on the input ID info? or we could skip redis for now - i'm not sure whether going to redis on every request will be significantly faster than going to the db on every request

in any case, i think we should try to avoid the duplicated implementation of the feature bucketing logic if we can. is there a reason why we can't create a Feature() and call .check_value()? its caching may not be useful, but i don't think it would be harmful

codecov/settings_base.py Outdated Show resolved Hide resolved
@daniel-codecov
Copy link
Contributor Author

rather than storing the FeatureFlag in a redis cache, could you go a layer higher and store the list of flag results in the cache based on the input ID info?

I feel like if we cache based on the user info, we would basically never get a cache hit since gazebo should only hit the endpoint at some interval of like 5 min

@daniel-codecov
Copy link
Contributor Author

in any case, i think we should try to avoid the duplicated implementation of the feature bucketing logic if we can. is there a reason why we can't create a Feature() and call .check_value()? its caching may not be useful, but i don't think it would be harmful

I can make it work, just need to do a minor rework of the class

matt-codecov
matt-codecov previously approved these changes May 1, 2024
@codecov-qa
Copy link

codecov-qa bot commented May 1, 2024

Codecov Report

Attention: Patch coverage is 95.29412% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 91.51%. Comparing base (8601246) to head (f9f3274).

✅ All tests successful. No failed tests found.

Files Patch % Lines
api/internal/feature/views.py 96.49% 2 Missing ⚠️
api/internal/feature/helpers.py 92.85% 1 Missing ⚠️
codecov/admin.py 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #531      +/-   ##
==========================================
+ Coverage   91.49%   91.51%   +0.01%     
==========================================
  Files         599      602       +3     
  Lines       16268    16352      +84     
==========================================
+ Hits        14885    14965      +80     
- Misses       1383     1387       +4     
Flag Coverage Δ
unit 91.51% <95.29%> (+0.01%) ⬆️
unit-latest-uploader 91.51% <95.29%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

Codecov Report

Attention: Patch coverage is 95.29412% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 91.51%. Comparing base (8601246) to head (f9f3274).

✅ All tests successful. No failed tests found ☺️

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #531      +/-   ##
==========================================
+ Coverage   91.49%   91.51%   +0.01%     
==========================================
  Files         599      602       +3     
  Lines       16268    16352      +84     
==========================================
+ Hits        14885    14965      +80     
- Misses       1383     1387       +4     
Flag Coverage Δ
unit 91.51% <95.29%> (+0.01%) ⬆️
unit-latest-uploader 91.51% <95.29%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
api/internal/feature/serializers.py 100.00% <100.00%> (ø)
api/internal/urls.py 97.29% <100.00%> (+0.07%) ⬆️
api/internal/feature/helpers.py 92.85% <92.85%> (ø)
codecov/admin.py 91.11% <75.00%> (-1.75%) ⬇️
api/internal/feature/views.py 96.49% <96.49%> (ø)

Impacted file tree graph

@daniel-codecov daniel-codecov added this pull request to the merge queue May 1, 2024
Merged via the queue into main with commit 03b5754 May 1, 2024
17 of 19 checks passed
@daniel-codecov daniel-codecov deleted the daniel/feature-endpoint branch May 1, 2024 22:41
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