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

[Rollout Infra] Start reading the GCS feature flags and expose them in a global Var #4869

Merged
merged 9 commits into from
Jan 23, 2025
1 change: 1 addition & 0 deletions server/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,7 @@ def create_app(nl_root=DEFAULT_NL_ROOT):
"config/home_page/partners.json")
app.config['HOMEPAGE_SAMPLE_QUESTIONS'] = libutil.get_json(
"config/home_page/sample_questions.json")
app.config['FEATURE_FLAGS'] = libutil.load_feature_flags()

if cfg.TEST or cfg.LITE:
app.config['MAPS_API_KEY'] = ''
Expand Down
25 changes: 25 additions & 0 deletions server/lib/feature_flags.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# Copyright 2020 Google LLC
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
"""Common library for functions related to feature flags"""

from flask import current_app

AUTOCOMPLETE_FEATURE_FLAG = 'autocomplete'
PLACE_PAGE_EXPERIMENT_FEATURE_FLAG = 'dev_place_experiment'
PLACE_PAGE_GA_FEATURE_FLAG = 'dev_place_ga'


def is_feature_enabled(feature_name: str) -> bool:
"""Returns whether the feature with `feature_name` is enabled."""
return current_app.config['FEATURE_FLAGS'].get(feature_name, False)
43 changes: 43 additions & 0 deletions server/lib/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
from flask import jsonify
from flask import make_response
from flask import request
from google.cloud import storage
from google.cloud.exceptions import NotFound
from google.protobuf import text_format

from server.config import subject_page_pb2
Expand Down Expand Up @@ -390,6 +392,47 @@ def get_nl_no_percapita_vars():
return nopc_vars


def get_feature_flag_bucket_name() -> str:
"""Returns the bucket name containing the feature flags."""
env_for_bucket = os.environ.get('FLASK_ENV')
if env_for_bucket in ['local', 'integration_test', 'test', 'webdriver']:
gmechali marked this conversation as resolved.
Show resolved Hide resolved
env_for_bucket = 'autopush'
elif env_for_bucket == 'production':
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also think about what to do for the 'staging' environment and various legacy custom data commons (e.g., 'feedingamerica', and others defined in server/app_env/*.py)

For staging, i think we should read the 'prod' bucket.
For the legacy custom dcs, we can either read the prod bucket (but that assumes the right read permissions are in place), or maybe we can just read a local feature flag file. Another option could be we keep the prod feature flag file checked into this repo, and for all other environments beyond what you have listed here, we just read that locally checked in version? wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would argue that staging should still read its own bucket, but that we should aim to keep the staging flags and prod flags in sync. They should only differ if we need to test something in staging before launching it to prod. So temporary diffs.
Do you agree?

For custom dc, I agree with your take that it should read the local file. If custom DC customers want to support launches on the fly, they can make their own bucket, store their flag and update the target. I ll leave that conversation for the following PR where I actually add the local files!

env_for_bucket = 'prod'
return 'datcom-website-' + env_for_bucket + '-resources'


def load_feature_flags():
"""Loads the feature flags into app config."""
storage_client = storage.Client()
bucket_name = get_feature_flag_bucket_name()
try:
bucket = storage_client.get_bucket(bucket_name)
except NotFound:
logging.error("Bucket not found: " + bucket_name)
return {}

blob = bucket.get_blob("feature_flags.json")
Copy link
Contributor

Choose a reason for hiding this comment

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

this might need a try/except block too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so actually if you look at the method implementation, it already nests the read call within a try/catch and returns None if there's a failure.
So this should never raise any exception.

data = {}
if blob:
try:
data = json.loads(blob.download_as_bytes())
except json.JSONDecodeError:
logging.warning("Loading feature flags failed to decode JSON.")
except TypeError:
logging.warning("Loading feature flags encountered a TypeError.")
else:
logging.warning("Feature flag file not found in the bucket.")

# Create the dictionary using a dictionary comprehension
feature_flag_dict = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just store the feature flag configuration as a object/dictionary in json?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I did load it up in json but doing this just plays it a little safer since we now know that if we have a feature flag, it must have a name and enabled.
so in case the file that was read in had bad data this would ensure it doesn't get added to the flags.

flag["name"]: flag["enabled"]
for flag in data
if 'name' in flag and 'enabled' in flag
}
return feature_flag_dict


# Returns a set of SVs that have percentage units.
# (Generated from http://gpaste/6422443047518208)
def get_sdg_percent_vars():
Expand Down
10 changes: 9 additions & 1 deletion server/routes/place/html.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@

from server.lib.cache import cache
from server.lib.config import GLOBAL_CONFIG_BUCKET
from server.lib.feature_flags import is_feature_enabled
from server.lib.feature_flags import PLACE_PAGE_EXPERIMENT_FEATURE_FLAG
from server.lib.feature_flags import PLACE_PAGE_GA_FEATURE_FLAG
from server.lib.i18n import AVAILABLE_LANGUAGES
from server.lib.i18n import DEFAULT_LOCALE
import server.routes.dev_place.utils as utils
Expand Down Expand Up @@ -302,6 +305,9 @@ def is_seo_experiment_enabled(place_dcid: str, category: str,
def is_dev_place_experiment_enabled(place_dcid: str, locale: str,
request_args: MultiDict[str, str]) -> bool:
"""Determine if dev place experiment should be enabled for the page"""
if not is_feature_enabled(PLACE_PAGE_EXPERIMENT_FEATURE_FLAG):
return False

# Disable dev place experiment for non-dev environments
# TODO(dwnoble): Remove this before prod release
if os.environ.get('FLASK_ENV') not in [
Expand All @@ -326,7 +332,9 @@ def is_dev_place_experiment_enabled(place_dcid: str, locale: str,
@bp.route('/<path:place_dcid>')
@cache.cached(query_string=True)
def place(place_dcid=None):
if is_dev_place_experiment_enabled(place_dcid, g.locale, flask.request.args):
if is_feature_enabled(
PLACE_PAGE_GA_FEATURE_FLAG) or is_dev_place_experiment_enabled(
place_dcid, g.locale, flask.request.args):
return dev_place(place_dcid=place_dcid)
redirect_args = dict(flask.request.args)

Expand Down
1 change: 1 addition & 0 deletions server/templates/base.html
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@
<script>
globalThis.isCustomDC = {{ config['CUSTOM']|int }};
globalThis.STAT_VAR_HIERARCHY_CONFIG = {{ config['STAT_VAR_HIERARCHY_CONFIG'] | tojson }};
globalThis.FEATURE_FLAGS = {{ config['FEATURE_FLAGS'] | tojson }};
globalThis.enableBQ = {{ config['ENABLE_BQ']|int }};
</script>
<script>
Expand Down
20 changes: 15 additions & 5 deletions static/js/components/nl_search_bar/nl_search_bar_header_inline.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,12 @@
* Inline-header version of the NL Search Component - used in Version 2 of the header
*/

import React, { ReactElement } from "react";
import React, { ReactElement, useEffect, useState } from "react";

import {
AUTOCOMPLETE_FEATURE_FLAG,
isFeatureEnabled,
} from "../../shared/feature_flags/util";
import { NlSearchBarImplementationProps } from "../nl_search_bar";
import { AutoCompleteInput } from "./auto_complete_input";

Expand All @@ -32,16 +36,22 @@ const NlSearchBarHeaderInline = ({
onSearch,
shouldAutoFocus,
}: NlSearchBarImplementationProps): ReactElement => {
const [autocompleteEnabled, setAutoCompleteEnabled] = useState(false);
const urlParams = new URLSearchParams(window.location.search);
const isAutopushEnv = window.location.hostname == "autopush.datacommons.org";
const enableAutoComplete =
isAutopushEnv ||
(urlParams.has("ac_on") && urlParams.get("ac_on") == "true");

useEffect(() => {
setAutoCompleteEnabled(
isFeatureEnabled(AUTOCOMPLETE_FEATURE_FLAG) ||
isAutopushEnv ||
gmechali marked this conversation as resolved.
Show resolved Hide resolved
(urlParams.has("ac_on") && urlParams.get("ac_on") == "true")
);
}, []);

return (
<div className="header-search-section">
<AutoCompleteInput
enableAutoComplete={enableAutoComplete}
enableAutoComplete={autocompleteEnabled}
value={value}
invalid={invalid}
placeholder={placeholder}
Expand Down
32 changes: 32 additions & 0 deletions static/js/shared/feature_flags/util.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/**
* Copyright 2025 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

export const FEATURE_FLAGS = globalThis.FEATURE_FLAGS;
export const AUTOCOMPLETE_FEATURE_FLAG = "autocomplete";
export const PLACE_PAGE_EXPERIMENT_FEATURE_FLAG = "autocomplete";
export const PLACE_PAGE_GA_FEATURE_FLAG = "autocomplete";

/**
* Helper method to interact with feature flags.
* @param featureName name of feature for which we want status.
* @returns Bool describing if the feature is enabled
*/
export function isFeatureEnabled(featureName: string): boolean {
if (featureName in FEATURE_FLAGS) {
return FEATURE_FLAGS[featureName];
}
return false;
}
Loading