From 26ffb931c99cce0fb9eb0aa01cf955510db69525 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Fri, 27 Sep 2024 08:38:11 -0700 Subject: [PATCH 1/7] optimize S3 partitioning --- app/aws/s3.py | 35 ++++++++++++++++++++++++++++------- 1 file changed, 28 insertions(+), 7 deletions(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index 52e2a5eb1..f358a13f3 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -12,6 +12,7 @@ from app.clients import AWS_CLIENT_CONFIG from notifications_utils import aware_utcnow FILE_LOCATION_STRUCTURE = "service-{}-notify/{}.csv" +NEW_FILE_LOCATION_STRUCTURE = "{}-service-notify/{}.csv" # Temporarily extend cache to 7 days ttl = 60 * 60 * 24 * 7 @@ -211,6 +212,21 @@ def file_exists(file_location): def get_job_location(service_id, job_id): + return ( + current_app.config["CSV_UPLOAD_BUCKET"]["bucket"], + NEW_FILE_LOCATION_STRUCTURE.format(service_id, job_id), + current_app.config["CSV_UPLOAD_BUCKET"]["access_key_id"], + current_app.config["CSV_UPLOAD_BUCKET"]["secret_access_key"], + current_app.config["CSV_UPLOAD_BUCKET"]["region"], + ) + + +def get_old_job_location(service_id, job_id): + """ + This is deprecated. We are transitioning to NEW_FILE_LOCATION_STRUCTURE, + but it will take a few days where we have to support both formats. + Remove this when everything works with the NEW_FILE_LOCATION_STRUCTURE. + """ return ( current_app.config["CSV_UPLOAD_BUCKET"]["bucket"], FILE_LOCATION_STRUCTURE.format(service_id, job_id), @@ -239,9 +255,11 @@ def get_job_from_s3(service_id, job_id): max_retries = 4 backoff_factor = 0.2 - if not file_exists(FILE_LOCATION_STRUCTURE.format(service_id, job_id)): + if not file_exists( + FILE_LOCATION_STRUCTURE.format(service_id, job_id) + ) and not file_exists(NEW_FILE_LOCATION_STRUCTURE.format(service_id, job_id)): current_app.logger.error( - f"This file does not exist {FILE_LOCATION_STRUCTURE.format(service_id, job_id)}" + f"This file with service_id {service_id} and job_id {job_id} does not exist" ) return None @@ -249,6 +267,9 @@ def get_job_from_s3(service_id, job_id): try: obj = get_s3_object(*get_job_location(service_id, job_id)) + # TODO remove this when we have fully transitioned to the NEW_FILE_LOCATION_STRUCTURE + if obj is None: + obj = get_s3_object(*get_old_job_location(service_id, job_id)) return obj.get()["Body"].read().decode("utf-8") except botocore.exceptions.ClientError as e: if e.response["Error"]["Code"] in [ @@ -257,7 +278,7 @@ def get_job_from_s3(service_id, job_id): "SlowDown", ]: current_app.logger.exception( - f"Retrying job fetch {FILE_LOCATION_STRUCTURE.format(service_id, job_id)} retry_count={retries}", + f"Retrying job fetch retry_count={retries}", ) retries += 1 sleep_time = backoff_factor * (2**retries) # Exponential backoff @@ -266,18 +287,18 @@ def get_job_from_s3(service_id, job_id): else: # Typically this is "NoSuchKey" current_app.logger.exception( - f"Failed to get job {FILE_LOCATION_STRUCTURE.format(service_id, job_id)}", + f"Failed to get job with service_id {service_id} job_id {job_id}", ) return None except Exception: current_app.logger.exception( - f"Failed to get job {FILE_LOCATION_STRUCTURE.format(service_id, job_id)} retry_count={retries}", + f"Failed to get job with service_id {service_id} job_id {job_id}retry_count={retries}", ) return None current_app.logger.error( - f"Never retrieved job {FILE_LOCATION_STRUCTURE.format(service_id, job_id)}", + f"Never retrieved job with service_id {service_id} job_id {job_id}", ) return None @@ -358,7 +379,7 @@ def get_phone_number_from_s3(service_id, job_id, job_row_number): if job is None: current_app.logger.error( - f"Couldnt find phone for job {FILE_LOCATION_STRUCTURE.format(service_id, job_id)} because job is missing" + f"Couldnt find phone for job with service_id {service_id} job_id {job_id} because job is missing" ) return "Unavailable" From ca89779a524eb5abc11feb76ecdd95167509ca9b Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Fri, 27 Sep 2024 08:52:11 -0700 Subject: [PATCH 2/7] get the lines that are not covered for tests --- .github/workflows/checks.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index 14128b0f8..05f044d9d 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -63,7 +63,7 @@ jobs: NOTIFY_E2E_TEST_PASSWORD: ${{ secrets.NOTIFY_E2E_TEST_PASSWORD }} - name: Check coverage threshold # TODO get this back up to 95 - run: poetry run coverage report --fail-under=95 + run: poetry run coverage report -m --fail-under=95 validate-new-relic-config: runs-on: ubuntu-latest From 8918dbcffdb70dbdef71cf16a570f7bf36740c99 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Fri, 27 Sep 2024 09:07:02 -0700 Subject: [PATCH 3/7] increase test coverage --- app/commands.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/commands.py b/app/commands.py index 907bbe7cf..1cf8379cb 100644 --- a/app/commands.py +++ b/app/commands.py @@ -931,7 +931,7 @@ where possible to enable better maintainability. # generate n number of test orgs into the dev DB @notify_command(name="add-test-organizations-to-db") @click.option("-g", "--generate", required=True, prompt=True, default=1) -def add_test_organizations_to_db(generate): +def add_test_organizations_to_db(generate): # pragma: no cover if getenv("NOTIFY_ENVIRONMENT", "") not in ["development", "test"]: current_app.logger.error("Can only be run in development") return @@ -993,7 +993,7 @@ def add_test_organizations_to_db(generate): # generate n number of test services into the dev DB @notify_command(name="add-test-services-to-db") @click.option("-g", "--generate", required=True, prompt=True, default=1) -def add_test_services_to_db(generate): +def add_test_services_to_db(generate): # pragma: no cover if getenv("NOTIFY_ENVIRONMENT", "") not in ["development", "test"]: current_app.logger.error("Can only be run in development") return @@ -1007,7 +1007,7 @@ def add_test_services_to_db(generate): # generate n number of test jobs into the dev DB @notify_command(name="add-test-jobs-to-db") @click.option("-g", "--generate", required=True, prompt=True, default=1) -def add_test_jobs_to_db(generate): +def add_test_jobs_to_db(generate): # pragma: no cover if getenv("NOTIFY_ENVIRONMENT", "") not in ["development", "test"]: current_app.logger.error("Can only be run in development") return @@ -1022,7 +1022,7 @@ def add_test_jobs_to_db(generate): # generate n number of notifications into the dev DB @notify_command(name="add-test-notifications-to-db") @click.option("-g", "--generate", required=True, prompt=True, default=1) -def add_test_notifications_to_db(generate): +def add_test_notifications_to_db(generate): # pragma: no cover if getenv("NOTIFY_ENVIRONMENT", "") not in ["development", "test"]: current_app.logger.error("Can only be run in development") return @@ -1043,7 +1043,7 @@ def add_test_notifications_to_db(generate): @click.option("-g", "--generate", required=True, prompt=True, default="1") @click.option("-s", "--state", default="active") @click.option("-d", "--admin", default=False, type=bool) -def add_test_users_to_db(generate, state, admin): +def add_test_users_to_db(generate, state, admin): # pragma: no cover if getenv("NOTIFY_ENVIRONMENT", "") not in ["development", "test"]: current_app.logger.error("Can only be run in development") return From d6f4acaa561ff31fc55bc333f05f747e4097835b Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Fri, 27 Sep 2024 09:10:51 -0700 Subject: [PATCH 4/7] fix format --- app/commands.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/commands.py b/app/commands.py index 1cf8379cb..45fce9211 100644 --- a/app/commands.py +++ b/app/commands.py @@ -1043,7 +1043,7 @@ def add_test_notifications_to_db(generate): # pragma: no cover @click.option("-g", "--generate", required=True, prompt=True, default="1") @click.option("-s", "--state", default="active") @click.option("-d", "--admin", default=False, type=bool) -def add_test_users_to_db(generate, state, admin): # pragma: no cover +def add_test_users_to_db(generate, state, admin): # pragma: no cover if getenv("NOTIFY_ENVIRONMENT", "") not in ["development", "test"]: current_app.logger.error("Can only be run in development") return From 6b861f31d70e1a5f658d1ced7281be343083c090 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Mon, 30 Sep 2024 08:45:15 -0700 Subject: [PATCH 5/7] fix debug --- app/aws/s3.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index f358a13f3..cc25f168a 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -278,7 +278,7 @@ def get_job_from_s3(service_id, job_id): "SlowDown", ]: current_app.logger.exception( - f"Retrying job fetch retry_count={retries}", + f"Retrying job fetch service_id {service_id} job_id {job_id} retry_count={retries}", ) retries += 1 sleep_time = backoff_factor * (2**retries) # Exponential backoff From 97ee4fe03200fe07532f08dcb361e16bc28ee331 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 2 Oct 2024 07:31:13 -0700 Subject: [PATCH 6/7] cleanup --- app/aws/s3.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index c652e70de..bd0301d78 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -318,11 +318,17 @@ def get_job_from_s3(service_id, job_id): while retries < max_retries: try: - obj = get_s3_object(*get_job_location(service_id, job_id)) - # TODO remove this when we have fully transitioned to the NEW_FILE_LOCATION_STRUCTURE - if obj is None: + # TODO + # for transition on optimizing the s3 partition, we have + # to check for the file location using the new way and the + # old way. After this has been on production for a few weeks + # we should remove the check for the old way. + try: + obj = get_s3_object(*get_job_location(service_id, job_id)) + return obj.get()["Body"].read().decode("utf-8") + except botocore.exceptions.ClientError: obj = get_s3_object(*get_old_job_location(service_id, job_id)) - return obj.get()["Body"].read().decode("utf-8") + return obj.get()["Body"].read().decode("utf-8") except botocore.exceptions.ClientError as e: if e.response["Error"]["Code"] in [ "Throttling", From c792a2492d41cd543548f997b9540e04ac887d89 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 2 Oct 2024 07:58:24 -0700 Subject: [PATCH 7/7] cleanup --- tests/app/aws/test_s3.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/aws/test_s3.py b/tests/app/aws/test_s3.py index 8e3863d5c..e468c4426 100644 --- a/tests/app/aws/test_s3.py +++ b/tests/app/aws/test_s3.py @@ -151,7 +151,7 @@ def test_get_job_from_s3_exponential_backoff_on_throttling(mocker): mocker.patch("app.aws.s3.file_exists", return_value=True) job = get_job_from_s3("service_id", "job_id") assert job is None - assert mock_get_object.call_count == 4 + assert mock_get_object.call_count == 8 def test_get_job_from_s3_exponential_backoff_file_not_found(mocker):