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

Deprecate old feature flags #9779

Closed
agjohnson opened this issue Dec 6, 2022 · 11 comments
Closed

Deprecate old feature flags #9779

agjohnson opened this issue Dec 6, 2022 · 11 comments
Assignees
Labels
Improvement Minor improvement to code

Comments

@agjohnson
Copy link
Contributor

agjohnson commented Dec 6, 2022

With #9290 we removed some of the advertised feature flags we had added for users. There are still a few old feature flags kicking around that we might want to review more, and should remove what we think makes sense 1. While it would be ideal to remove all old feature flags, the ones most ripe for removal are:

  • Feature flags that are commonly requested or incurred in support
  • Feature flags that have a fair amount of projects opted in
  • Feature flags that were meant to be user controlled.

The remaining feature flags we currently have are here:

https://github.com/readthedocs/readthedocs.org/blob/6e42d46/readthedocs/projects/models.py#L1798-L1844

For new beta features and new feature flags, the pattern we're moving back to is:

  • We will use a feature flag in our application when:
    • We need to guard the feature against wide use
    • We are testing a major change and we don't want users to have to opt in to a feature
    • We are hotfixing something or providing a feature dependent on a timeframe
  • We will not use a feature flag:
    • To provide temporary/testing features. Users should opt-in to features with the configuration file and keep these requests out of support
    • To avoid backwards compatibility or to avoid changing our configuration file prematurely

So, with that in mind, any old feature flag that seems major enough should be deprecated and should become a user-controllable configuration option.

I opened up #9698 to discuss configuration and opt-in for temporary/beta features. I think this solves the middle ground, where we want to develop and test a feature, but we need to communicate that the feature is beta quality still.

Footnotes

  1. I would skip feature flags that are not heavily used and any feature that would be a sideways move. The goal is to give more control to users and keep user requests out of our support inbox. Going forward, I think a steady pattern for feature flags helps us the most.

@humitos
Copy link
Member

humitos commented Jun 12, 2023

I jumped into production's Django shell and create a summary of each feature flag. I will start creating PRs with the ones that I think we can remove without any kind of notification/plan. Feel free to comment here if you have different opinions or what to do on each case.

Feature flag Community (projects, past, future) Commercial (projects, past, future) Comment
use_sphinx_builders 99823, True, True 1, True, True Removing it since it's enabled for everybody and it's not used.
deduplicate_builds 72349, True, True 3, False, False We should activate it for everybody in commercial, and remove it completely after some weeks.
using_github_webhook_1 ... ... Already removed from code, but still on the db. We should remove the row to clean it up.
use_sphinx_latest 543, False, True 1, False, True We plan to remove this on #10365
using_github_webhook_2 523, False, False ... Removed from code. It should be safe to remove.
using_bitbucket_webhook_1 82, False, False ... Removed from code. It should be safe to remove.
using_generic_webhook_1 65, False, False ... Removed from code. It should be safe to remove.
dont_shallow_clone 60, False, False 1, False, False This is possible via build.jobs now. We should communicate that to these users and remove it after that.
allow_forced_redirects 34, False, False 3, False, False This one seems valid still. We should talk about if it's possible to enable it to all projects or that will kill our servers?
sphinx_parallel 32, False, False ... Failed attempt to make Sphinx run fast. We should probably remove it completely without any notification to users.
dont_overwrite_sphinx_context 30, True, True 15, True, True Enabled by default to everybody. We can remove it without issues.
clean_after_build ... ... This became a Django setting and it's enabled by default. We should delete it from code and db.
conda_append_core_requirements 20, True, True 4, False, False We should enable by default on commercial and remove it from code and db.
resolve_project_from_header 19, False, False 1, False, False This is an unsupported feature. We should contact the users and tell them to migrate.
cancel_old_builds 19, True, True 0, True, True We can safely remove this. It's enabled by default to everybody.
conda_uses_mamba ... ... It's not in the code anymore. We should remove it from the db.
disable_server_side_search 13, False, False 12, False, False Not sure why we need this. I suppose it's to protect ourselves on some big projects?
list_packages_installed_env 9, False, False 1, False, False This can now be achieved with build.jobs. We can safely remove it without any communication I'd say.
update_conda_startup 8, False, False 5, False, False I think this is not required anymore and we can remove it. People could update Conda with build.jobs if they really need it.
use_unresolver_with_proxito 6, True, True 4, False, False Required while we do the migration to the new unresolver, I guess.
use_pdf_latexmk 6, False, False 2, False, False Not in code. Can be removed from db.
pip_always_upgrade 5, False, False ... Safe to remove. It can be achieved with build.jobs.pre_create_environment.
vcs_remote_listing 5, True, True 1, True, True Safe to remove. Default to everybody.
skip_sync_tags 3, False, False 3, False, False We used it in the past to protect ourselves from projects with thousands of Git tags. May be useful to keep it, I guess.
record_404_page_views 3, False, False 0, True, True I don't think we can enabled it community due the amount of traffic there. Probably smart to keep it. Otherwise, we should move the logic application itself and remove the feature flag.
use_rclone 2, True, True 1, True, True Safe to remove. Enabled by default everywhere.
default_to_fuzzy_search 2, False, False 8, False, False Not sure what it means. Probably replaced by Addons Search filters/checkboxes in the future?
using_gitlab_webhook_1 2, False, False ... Not in code. Safe to remove.
cached_environment 2, False, False 2, False, False Failed attempt to cache environments. Probably worth removing this.
update_ca_certificates 1, True, True ... Can be removed from db. It does not appear in code.
dont_create_index 1, False, False 1, False, False Why do we need this? Probably worth removing it as well. I think we should stop creating the index for everybody, tho 😄
allow_deprecated_webhooks 1, False, False 0, True, False Not used in code. Only leftovers.
gvisor_runtime 1, False, False ... I thought we were using this in production for everybody 🤷. cc @agjohnson
use_testing_build_image 1, False, False ... Worth to remove it.
default_to_mkdocs_0_17_3 0, True, False 0, True, False I don't think we can remove it without a deprecation plan with notifications. Similar to #10365.
use_mkdocs_latest 0, False, True 0, False, True The logic for this feature flag is wrong: #10402. Not sure what to do here.
limit_concurrent_builds 0, True, True 2, True, True Safe to remove. Enabled for everybody.
mkdocs_theme_rtd ... 197, False, False Not sure why we need this. There is a note at
# README: historically, the default theme was ``readthedocs`` but in
# https://github.com/rtfd/readthedocs.org/pull/4556 we change it to
# ``mkdocs`` to maintain the same behavior in Read the Docs than
# building locally. Although, we can't apply this into the Corporate
# site. To keep the same default theme there, we created a Feature flag
# for these project that were building with MkDocs in the Corporate
# site.
use_old_custom_domain_cname ... 20, False, False We may be able to remove this if we have migrated all the custom domains to the new pattern.
cdn_enabled ... 13, False, False It seems is has been integrated by default in the app. It's safe to remove.
api_large_data ... 6, False, False A way to protect ourselves for projects that send a lot of data via the builder's API.
skip_sync_versions ... 6, False, False A way to protect ourselves from projects with an insane amount of tags/branches.
rtd_sphinx_ext_latest ... 3, False, False I'm not sure why we wouldn't want the latest -ext here? In any case, this will be removed once we have the addons in place.
enable_mkdocs_server_side_search ... 2, False, False Not used in code. The leftovers can be removed and the db entry too.
auth_header_session ... 1, False, False I guess this was a POC when working with Auth0 or similar.
index_from_html_files ... 1, False, False This can be removed after implementing #10272.
use_custom_domain_sha_as_cname ... 0, True, False Same as use_old_custom_domain_cname.

The query used to get this data was:

list(Feature.objects.annotate(n_projects=Count("projects")).filter(Q(n_projects__gt=0) | Q(default_true=True) | Q(future_default_true=True)).order_by("-n_projects").values_list("feature_id", "n_projects", "default_true", "future_default_true"))

@humitos humitos moved this from Planned to In progress in 📍Roadmap Jun 12, 2023
@humitos
Copy link
Member

humitos commented Jun 12, 2023

Feature flags with 0 projects using them and default_true=False and future_default_true=False. So, nobody are using them

Feature flag Comment
hosting_integrations This is implemented at the Version level now. It can be removed I guess.
disable_pageviews Probably a protection for ourselves on big projects. May be worth to keep it for now.
disable_sphinx_domains I think it's gonna be removed by #10272.

@stsewd
Copy link
Member

stsewd commented Jun 13, 2023

disable_server_side_search

This is for projects that don't want to use our server side search.

default_to_fuzzy_search

This offers fuzzy search by default for projects, this is useful to get partial matches, instead of having to match an exact word, it does slow down search a bit, so it's under a feature flag.

rtd_sphinx_ext_latest, I'm not sure why we wouldn't want the latest -ext here?

This is useful when releasing a breaking change, or if we want to test something, and we are not sure if it will work for all projects

@humitos
Copy link
Member

humitos commented Jun 13, 2023

disable_server_side_search

This is for projects that don't want to use our server side search.

I understand this feature flag will be able to be removed once we have addons in place as well, since we won't be injecting/integrating the Sphinx search input with our own search 👍🏼

@ericholscher
Copy link
Member

Some patterns we've discovered:

  • Send email to all projects once, then ongoing emails for active projects
  • Make it obvious to users when they have succeed in the migration (eg. a Success! message on the build page)

@humitos
Copy link
Member

humitos commented Jun 17, 2023

My next step here is to update the table from my previous comment crossing-out the ones we removed, and updating the comments as needed. After that, we can discuss how we want to remove the ones that require a deprecation path and their priorities.

Also, I want to update our meta discussion https://github.com/readthedocs/meta/discussions/68 with the learning from the current deprecation it's going on and define/set a generic plan for small and big impact deprecation.

humitos added a commit that referenced this issue Jun 22, 2023
There are some leftovers here. CDN is enabled by default to all projects.

Related #9779
@humitos
Copy link
Member

humitos commented Jun 22, 2023

This is the updated version of the table from #9779 (comment) without the ones that were removed already.

Feature flag Community (projects, past, future) Commercial (projects, past, future) Comment
use_sphinx_latest 543, False, True 1, False, True We plan to remove this on #10365. Deprecated on #10508
dont_shallow_clone 60, False, False 1, False, False This is possible via build.jobs now. We should communicate that to these users and remove it after that.
allow_forced_redirects 34, False, False 3, False, False This one seems valid still. We should talk about if it's possible to enable it to all projects or that will kill our servers?
conda_append_core_requirements 20, True, True 4, False, False We should enable by default on commercial and remove it from code and db. See #10477
resolve_project_from_header 19, False, False 1, False, False This is an unsupported feature. We should contact the users and tell them to migrate.
disable_server_side_search 13, False, False 12, False, False This can be removed once we implement the Search Addon with the new .js client.
update_conda_startup 8, False, False 5, False, False I think this is not required anymore and we can remove it. People could update Conda with build.jobs if they really need it.
use_unresolver_with_proxito 6, True, True 4, False, False Required while we do the migration to the new unresolver, I guess.
skip_sync_tags 3, False, False 3, False, False We used it in the past to protect ourselves from projects with thousands of Git tags. May be useful to keep it, I guess.
record_404_page_views 3, False, False 0, True, True I don't think we can enabled it community due the amount of traffic there. Probably smart to keep it. Otherwise, we should move the logic application itself and remove the feature flag.
default_to_fuzzy_search 2, False, False 8, False, False Useful for some projects at the cost of being slower. May be a good candidate for #9698. Maybe even better a checkbox/filter on the Search Addons?
dont_create_index 1, False, False 1, False, False Read my comment in #9290 (comment). Work is moving forward in #1800 (comment)
gvisor_runtime 1, False, False ... It's being used for build.jobs and build.commands. We should enable it for everybody for a month and delete it from our code and db.
default_to_mkdocs_0_17_3 0, True, False 0, True, False I don't think we can remove it without a deprecation plan with notifications. Similar to #10365. We have 274 projects using this version: https://ethicalads.metabaseapp.com/question/218-projects-using-mkdocs-group-by-version
use_mkdocs_latest 0, False, True 0, False, True The logic for this feature flag is wrong: #10402. Not sure what to do here.
mkdocs_theme_rtd ... 197, False, False Not sure why we need this. There is a note at
# README: historically, the default theme was ``readthedocs`` but in
# https://github.com/rtfd/readthedocs.org/pull/4556 we change it to
# ``mkdocs`` to maintain the same behavior in Read the Docs than
# building locally. Although, we can't apply this into the Corporate
# site. To keep the same default theme there, we created a Feature flag
# for these project that were building with MkDocs in the Corporate
# site.
api_large_data ... 6, False, False A way to protect ourselves for projects that send a lot of data via the builder's API.
skip_sync_versions ... 6, False, False A way to protect ourselves from projects with an insane amount of tags/branches.
rtd_sphinx_ext_latest ... 3, False, False Useful when releasing "breaking changes" in our extension. In any case, this will be removed once we have the addons in place.
auth_header_session ... 1, False, False I guess this was a POC when working with Auth0 or similar.
index_from_html_files ... 1, False, False This can be removed after implementing #10272.
use_old_custom_domain_cname ... 20, False, False We may be able to remove this if we have migrated all the custom domains to the new pattern. What happens when old domains pointing to <org-slug>.users.readthedocs.comauto-renew? Do they start pointing to the new domain? I guess no. What we should do with those? cc @stsewd @erichoslcher
use_custom_domain_sha_as_cname ... 0, True, False Same as use_old_custom_domain_cname.
hosting_integrations ... ... This is implemented at the Version level now. It will be useful to gradually roll-out the new Addons, tho.
disable_pageviews ... ... Probably a protection for ourselves on big projects. May be worth to keep it for now.

I will be taking one at a time and writing down each of the plans on Slack looking for approvals and I will implement the plan after that.

humitos added a commit that referenced this issue Jun 28, 2023
Example using `build.jobs.pre_create_environment` to update Conda.
We will link this example in the email we will be sending to users
when deprecating/removing `UPDATE_CONDA_STARTUP` feature flag.

See #9779
humitos added a commit that referenced this issue Jun 29, 2023
Example using `build.jobs.pre_create_environment` to update Conda.
We will link this example in the email we will be sending to users
when deprecating/removing `UPDATE_CONDA_STARTUP` feature flag.

See #9779
@humitos
Copy link
Member

humitos commented Jul 10, 2023

There is some work going on here: storing some logs in NR to be able to contact these users and also waiting for the exact date to be able to remove some of the features for what users were already contacted. I'm putting this issue in a future sprint so I can come back to it and do the final work.

humitos added a commit that referenced this issue Jul 31, 2023
We want to keep deprecating old feature flags we don't want to support forever.
In particular, those that keep our users using pretty old versions of doctools
and freeze them in a status that's hard to maintain.

This PR removes the `DEFATUL_TO_MKDOCS_0_17_3` feature flag and always install
the latest `mkdocs` version. Note this feature flag has `default_true=True` for
those projects created _before 2019-04-03_.

I did a small DB query and found:

```python
>>> Project.objects.filter(pub_date__lt=timezone.datetime(2019, 4, 3),
documentation_type='mkdocs').count()
7529
```

However, note we don't know if these projects are pinning a version of MkDocs,
so we don't exactly know how many of those are using this feature. However, it
gives us a maximum number at least.

On the other hand, taking a look at Metabase, using the build data from the past
6 months I found that we only have 293 projects:
https://ethicalads.metabaseapp.com/question/218-projects-using-mkdocs-group-by-version

Related #9779
@humitos
Copy link
Member

humitos commented Aug 1, 2023

This is the updated version of the table from #9779 (comment) without the ones that were removed already or are in the process.

Feature flag Comment
allow_forced_redirects This one seems valid still. We should talk about if it's possible to enable it to all projects or that will kill our servers?
resolve_project_from_header This is an unsupported feature. We should contact the users and tell them to migrate.
disable_server_side_search This can be removed once we implement the Search Addon with the new .js client.
use_unresolver_with_proxito Required while we do the migration to the new unresolver, I guess.
skip_sync_tags We used it in the past to protect ourselves from projects with thousands of Git tags. May be useful to keep it, I guess.
record_404_page_views I don't think we can enabled it community due the amount of traffic there. Probably smart to keep it. Otherwise, we should move the logic application itself and remove the feature flag.
default_to_fuzzy_search Useful for some projects at the cost of being slower. May be a good candidate for #9698. Maybe even better a checkbox/filter on the Search Addons?
mkdocs_theme_rtd Not sure why we need this. There is a note at
# README: historically, the default theme was ``readthedocs`` but in
# https://github.com/rtfd/readthedocs.org/pull/4556 we change it to
# ``mkdocs`` to maintain the same behavior in Read the Docs than
# building locally. Although, we can't apply this into the Corporate
# site. To keep the same default theme there, we created a Feature flag
# for these project that were building with MkDocs in the Corporate
# site.
api_large_data A way to protect ourselves for projects that send a lot of data via the builder's API.
skip_sync_versions A way to protect ourselves from projects with an insane amount of tags/branches.
rtd_sphinx_ext_latest Useful when releasing "breaking changes" in our extension. In any case, this will be removed once we have the addons in place.
auth_header_session I guess this was a POC when working with Auth0 or similar.
use_old_custom_domain_cname We may be able to remove this if we have migrated all the custom domains to the new pattern. What happens when old domains pointing to <org-slug>.users.readthedocs.comauto-renew? Do they start pointing to the new domain? I guess no. What we should do with those? cc @stsewd @erichoslcher
use_custom_domain_sha_as_cname Same as use_old_custom_domain_cname.
hosting_integrations This is implemented at the Version level now. It will be useful to gradually roll-out the new Addons, tho.
disable_pageviews Probably a protection for ourselves on big projects. May be worth to keep it for now.

I think there are no more feature flags we want to remove right now. I think we've done a really good work here by deprecating them, sending emails and moving forwards. The application is a lot more cleaner now and will allow us to move forward with more features.

I'm moving this issue to Q4 for now so we can continue with the clean up of those feature flags that are not high priority.

@stsewd
Copy link
Member

stsewd commented Aug 3, 2023

We may be able to remove this if we have migrated all the custom domains to the new pattern. What happens when old domains pointing to .users.readthedocs.comauto-renew? Do they start pointing to the new domain? I guess no. What we should do with those? cc @stsewd @erichoslcher

We need to contact each project and ask them to change the CNAME record to the new hashed one. I remember we contacted several of them in the past, so there are probably just a few using the old CNAME, we can also check which projects have the feature flag set.

@humitos
Copy link
Member

humitos commented Aug 9, 2023

@stsewd Do we have an issue for this already? If not, we should create one to track that particular work that involves extra steps. I'm closing this generic issue since we are not going to remove more feature flags for now.

@humitos humitos closed this as completed Aug 9, 2023
@github-project-automation github-project-automation bot moved this from In progress to Done in 📍Roadmap Aug 9, 2023
humitos added a commit that referenced this issue Sep 26, 2023
We want to keep deprecating old feature flags we don't want to support forever.
In particular, those that keep our users using pretty old versions of doctools
and freeze them in a status that's hard to maintain.

This PR removes the `DEFATUL_TO_MKDOCS_0_17_3` feature flag and always install
the latest `mkdocs` version. Note this feature flag has `default_true=True` for
those projects created _before 2019-04-03_.

I did a small DB query and found:

```python
>>> Project.objects.filter(pub_date__lt=timezone.datetime(2019, 4, 3),
documentation_type='mkdocs').count()
7529
```

However, note we don't know if these projects are pinning a version of MkDocs,
so we don't exactly know how many of those are using this feature. However, it
gives us a maximum number at least.

On the other hand, taking a look at Metabase, using the build data from the past
6 months I found that we only have 293 projects:
https://ethicalads.metabaseapp.com/question/218-projects-using-mkdocs-group-by-version

Related #9779
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Minor improvement to code
Projects
Archived in project
Development

No branches or pull requests

4 participants