Skip to content
This repository has been archived by the owner on May 6, 2024. It is now read-only.

feat: Output table sizes while calculating #7065

Closed
wants to merge 1 commit into from

Conversation

jdmulloy
Copy link
Contributor

Adds table sizes to output while running checks to add transparency to the process.

Configuration Pull Request

Make sure that the following steps are done before merging:

  • A SRE team member has approved the PR if it is code shared across multiple services and you don't own all of the services.
  • Are you adding any new default values that need to be overridden when this change goes live? If so:
    • Update the appropriate internal repo (be sure to update for all our environments)
    • If you are updating a secure value rather than an internal one, file a SRE ticket with details.
    • Add an entry to the CHANGELOG.
  • If you are making a complicated change, have you performed the proper testing specified on the Ops Ansible Testing Checklist? Adding a new variable does not require the full list (although testing on a sandbox is a great idea to ensure it links with your downstream code changes).
  • Think about how this change will affect Open edX operators. Have you updated the wiki page for the next Open edX release?

Adds table sizes to output while running checks to add transparency
to the process.
@jdmulloy
Copy link
Contributor Author

Adds output like this while checking each server

Checking table sizes for stage-edx-shared-002-us-east-1c.ciqreuddjk02.us-east-1.rds.amazonaws.com
RDS Name                                           Database Name                  Table Name                                                            
stage-edx-shared-002-us-east-1c                    reports_2_0                    module_engagement                                                                 41,022.00 MB
stage-edx-shared-002-us-east-1c                    reports_dev                    answer_distribution                                                               16,722.00 MB
stage-edx-shared-002-us-east-1c                    reports_emr_upgrade            answer_distribution                                                               12,908.00 MB
stage-edx-shared-002-us-east-1c                    reports_1_0                    student_engagement_wide_daily                                                     11,566.00 MB
stage-edx-shared-002-us-east-1c                    reports_emr_upgrade            course_enrollment_birth_year_daily                                                11,566.00 MB
stage-edx-shared-002-us-east-1c                    reports_1_0                    student_engagement_daily                                                           7,970.00 MB
stage-edx-shared-002-us-east-1c                    reports_2_0                    video_timeline                                                                     6,061.00 MB
stage-edx-shared-002-us-east-1c                    reports_1_0                    answer_distribution                                                                3,277.00 MB
stage-edx-shared-002-us-east-1c                    reports_1_0                    course_enrollment_birth_year_daily                                                 2,490.00 MB
stage-edx-shared-002-us-east-1c                    reports_2_0                    course_enrollment_mode_daily                                                       1,913.05 MB
stage-edx-shared-002-us-east-1c                    reports_emr_upgrade            course_enrollment_education_level_daily                                            1,569.00 MB
stage-edx-shared-002-us-east-1c                    reports_2_0                    course_enrollment_daily                                                            1,501.08 MB```

@jdmulloy jdmulloy requested review from adzuci and jmbowman November 16, 2023 18:06
Copy link
Contributor

@adzuci adzuci left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -76,6 +76,8 @@ def check_table_growth(rds_list, username, password, threshold, rds_threshold):
table_list = []
for db in rds_list:
print("Checking table sizes for {}".format(db["Endpoint"]))
format_string_header = "{:<50} {:<30} {:<70}"
print(format_string_header.format("RDS Name","Database Name", "Table Name", "Size"))
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: missing space after comma

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like that was copied from the code below for displaying the tables over the threshold, which would ideally also be fixed. Later, we should either add a linter here or move this code somewhere that already has linting set up.

@@ -102,12 +104,15 @@ def check_table_growth(rds_list, username, password, threshold, rds_threshold):
threshold_limit = threshold
for tables in rds_result:
temp_dict = {}
if tables[2] is not None and tables[2] > float(threshold_limit):
if tables[2] is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did we remove the and bit?

Copy link
Contributor

Choose a reason for hiding this comment

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

That condition got moved further down, so we can display the size of tables that don't exceed the threshold.

Copy link
Contributor

@jmbowman jmbowman left a comment

Choose a reason for hiding this comment

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

Would be nice to fix the formatting nit, but should be ok to go as is.

@@ -102,12 +104,15 @@ def check_table_growth(rds_list, username, password, threshold, rds_threshold):
threshold_limit = threshold
for tables in rds_result:
temp_dict = {}
if tables[2] is not None and tables[2] > float(threshold_limit):
if tables[2] is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

That condition got moved further down, so we can display the size of tables that don't exceed the threshold.

@@ -76,6 +76,8 @@ def check_table_growth(rds_list, username, password, threshold, rds_threshold):
table_list = []
for db in rds_list:
print("Checking table sizes for {}".format(db["Endpoint"]))
format_string_header = "{:<50} {:<30} {:<70}"
print(format_string_header.format("RDS Name","Database Name", "Table Name", "Size"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like that was copied from the code below for displaying the tables over the threshold, which would ideally also be fixed. Later, we should either add a linter here or move this code somewhere that already has linting set up.

@feanil feanil closed this May 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants