Conversation
![verdramvp[bot]](https://avatars.githubusercontent.com/u/67129199?s=60&v=4){kind=small}
There was a problem hiding this comment.
Suggested Code Quality Improvements
-
Slack2NRScript.py, line 10: Comments — The comment '#logging' is redundant as the following line of code clearly indicates that logging configuration is being set.
└─ Suggestion: Remove the comment '#logging'. -
Slack2NRScript.py, line 14: Comments — The commented-out code '#start service here' is unnecessary and does not provide any additional information or context.
└─ Suggestion: Remove the comment '#start service here'. -
Slack2NRScript.py, line 22: Naming — Variable names are defined as global strings but are populated later in different functions, making the code hard to follow and understand. It's unclear when and how these are supposed to be used just from reading the initialization.
└─ Suggestion: Avoid using global variables to store state. Consider encapsulating these properties in a class or a more structured manner. -
Slack2NRScript.py, line 25: Naming — The constant value '3600' is used for calculating time which is not immediately obvious as 'one hour' to someone unfamiliar with the context.
└─ Suggestion: Define a constant with a descriptive name, such as SECONDS_IN_AN_HOUR, and use that instead of the magic number '3600'. -
Slack2NRScript.py, line 69: Functions — The function 'fetch_from_newrelic' is doing multiple tasks: constructing queries, converting data format, and making HTTP requests, which violates the single responsibility principle.
└─ Suggestion: Split this function into smaller functions, each handling one specific part of the task such as building the query, making the HTTP request, and processing the response. -
Slack2NRScript.py, line 110: Structure — Deep nesting makes the code hard to read and maintain. In this instance, loops and conditionals are nested deeply within the function.
└─ Suggestion: Refactor to reduce nesting. For example, handle error cases early and return or continue, reducing the need for deep conditional blocks. -
Slack2NRScript.py, line 103: DRY — HTTP request code is repeated for different queries within the 'fetch_from_newrelic' function, violating the DRY principle.
└─ Suggestion: Extract the HTTP request logic into a separate function that can be reused for different queries. -
Slack2NRScript.py, line 123: Error Handling — HTTP requests might fail, but there's no specific error handling implemented for failed requests or erroneous data.
└─ Suggestion: Implement error handling for the HTTP request responses and verify their 'status' or 'data' content before processing them. -
Slack2NRScript.py, line 58: Expressiveness — Use of unexplained string formats and API keys directly in code.
└─ Suggestion: Extract literals like API keys and URL to constants at the beginning of the file or to a config file/environment variables for clarity and easier maintenance.
There was a problem hiding this comment.
Static Analysis Summary (In-House Scanner)
Db Calls In Loops (1)
IntegrationTester/Slack2NRScript.py, line 156:execute
Large Imports (1)
IntegrationTester/Slack2NRScript.py, line 11:basicConfig
Unused Functions (1)
IntegrationTester/Slack2NRScript.py, line 33:slack_event
Unused Variables (3)
IntegrationTester/Slack2NRScript.py, line 18:spreadsheet_idIntegrationTester/Slack2NRScript.py, line 20:clientIntegrationTester/Slack2NRScript.py, line 30:total_conversion
| #logging | ||
| logging.basicConfig(level=logging.INFO) | ||
|
|
There was a problem hiding this comment.
Comments — The comment '#logging' is redundant as the following line of code clearly indicates that logging configuration is being set.
Suggestion: Remove the comment '#logging'.
| logging.basicConfig(level=logging.INFO) | ||
|
|
||
| #service | ||
| #start service here |
There was a problem hiding this comment.
Comments — The commented-out code '#start service here' is unnecessary and does not provide any additional information or context.
Suggestion: Remove the comment '#start service here'.
| #app | ||
| app = Flask(__name__) | ||
|
|
||
| #populate variables | ||
| name = "" |
There was a problem hiding this comment.
Naming — Variable names are defined as global strings but are populated later in different functions, making the code hard to follow and understand. It's unclear when and how these are supposed to be used just from reading the initialization.
Suggestion: Avoid using global variables to store state. Consider encapsulating these properties in a class or a more structured manner.
| #app | ||
| app = Flask(__name__) | ||
|
|
||
| #populate variables |
There was a problem hiding this comment.
Naming — The constant value '3600' is used for calculating time which is not immediately obvious as 'one hour' to someone unfamiliar with the context.
Suggestion: Define a constant with a descriptive name, such as SECONDS_IN_AN_HOUR, and use that instead of the magic number '3600'.
| results_query = """{{actor {{ | ||
| account(id: SOME ACCOUNT ID) {{ | ||
| nrql( | ||
| query: "FROM Timer SELECT uniqueCount(CustomData_r1.artifact.id) as 'Unique Doc Count', count(CustomData_process.exit_code) as 'Conversion Attempts' WHERE Name = 'ConversionService.Complete' AND CustomData_process.exit_code IS NOT NULL AND CustomData_conversion.type.id = -1 AND CustomData_process.exit_code NOT IN (0, 4, 3328, 9, 11, 10, 59, 14) AND `CustomData_r1.tenant.name` = '{name}' LIMIT MAX FACET CustomData_process.exit_code, CustomData_message, CustomData_file.type SINCE '{start_time_str}' UNTIL '{end_time_str}'" | ||
| ) {{ | ||
| results | ||
| }} | ||
| }} | ||
| }} | ||
| }}""".format(name=name, start_time_str=start_time_str, end_time_str=end_time_str) | ||
|
|
||
| success_percentage_query = '''{{ | ||
| actor {{ | ||
| account(SOME ACCOUNT ID) {{ | ||
| nrql( | ||
| query: "SELECT latest(r1.source.name), latest(r1.ring.id), latest(cCnt) as 'Total Conversions', latest(sCnt) as 'Success Conversions', latest(sPer) as 'Success Percentage', latest(eCnt) as 'Error Conversoins', latest(ePer) as 'Error Percentage' FROM (SELECT count(CustomData_process.exit_code) as cCnt, filter(count(CustomData_process.exit_code), WHERE CustomData_process.exit_code IN (0, 4, 3328, 9, 11, 10, 59, 14)) as sCnt, percentage(count(*), WHERE CustomData_process.exit_code IN (0, 4, 3328, 9, 11, 10, 59, 14)) AS sPer, filter(count(CustomData_process.exit_code), WHERE CustomData_process.exit_code NOT IN (0, 4, 3328, 9, 11, 10, 59, 14)) as eCnt, percentage(count(*), WHERE CustomData_process.exit_code NOT IN (0, 4, 3328, 9, 11, 10, 59, 14)) AS ePer FROM Timer WHERE Name = 'ConversionService.Complete' AND CustomData_process.exit_code IS NOT NULL AND CustomData_conversion.type.id = -1 FACET CustomData_r1.tenant.name LIMIT MAX) JOIN ( SELECT r1.source.name, r1.ring.id, http.host FROM lookup(TenantLookup) ) ON `CustomData_r1.tenant.name` = http.host WHERE CustomData_r1.tenant.name = '{name}' FACET CustomData_r1.tenant.name LIMIT MAX SINCE '{start_time_str}' UNTIL '{end_time_str}'" | ||
| ) {{ | ||
| results | ||
| }} | ||
| }} | ||
| }} | ||
| }}'''.format(name=name, start_time_str=start_time_str, end_time_str=end_time_str) | ||
|
|
||
| headers = { | ||
| 'Accept': 'application/json', | ||
| 'Content-Type': 'application/json', | ||
| 'Api-Key' : "NRAK-16PL7JREKNJVUBNIMKVI0FMIICK" | ||
| } | ||
| SP_body = { | ||
| 'query': success_percentage_query, | ||
| 'accountID': 0000000 | ||
| } | ||
| results_body = { | ||
| 'query': results_query, | ||
| 'accountID': 0000000 | ||
| } | ||
| response = requests.post('https://api.newrelic.com/graphql', headers=headers, data = json.dumps(results_body)) | ||
| sp_response = requests.post('https://api.newrelic.com/graphql', headers=headers, data = json.dumps(SP_body)) | ||
| data = response.json() | ||
| sp_data = sp_response.json() | ||
| update_google_sheets(data, sp_data, end_time, name) | ||
|
|
||
| return | ||
| def update_google_sheets(data, sp_data,end_time,name): | ||
| # Convert the timestamp to a datetime object | ||
| SPREADSHEET_ID = '1PQCxTDftKu0nPicnGmLHWJbbkUp0UAFYO1X3STN3izY' # Replace with your Spreadsheet ID | ||
| SHEET_NAME = 'Sheet1' | ||
| last_row = get_last_row(service, SPREADSHEET_ID, SHEET_NAME) | ||
|
|
||
| sheet = service.spreadsheets() | ||
| result = sheet.values().get(spreadsheetId=SPREADSHEET_ID, range='Sheet1!A:A').execute() | ||
| rows = result.get('values', []) | ||
| next_row = len(rows) + 2 # Next row after the last populated cell, plus two for the space | ||
|
|
There was a problem hiding this comment.
Functions — The function 'fetch_from_newrelic' is doing multiple tasks: constructing queries, converting data format, and making HTTP requests, which violates the single responsibility principle.
Suggestion: Split this function into smaller functions, each handling one specific part of the task such as building the query, making the HTTP request, and processing the response.
| data = response.json() | ||
| sp_data = sp_response.json() | ||
| update_google_sheets(data, sp_data, end_time, name) | ||
|
|
There was a problem hiding this comment.
Structure — Deep nesting makes the code hard to read and maintain. In this instance, loops and conditionals are nested deeply within the function.
Suggestion: Refactor to reduce nesting. For example, handle error cases early and return or continue, reducing the need for deep conditional blocks.
| 'accountID': 0000000 | ||
| } | ||
| response = requests.post('https://api.newrelic.com/graphql', headers=headers, data = json.dumps(results_body)) | ||
| sp_response = requests.post('https://api.newrelic.com/graphql', headers=headers, data = json.dumps(SP_body)) | ||
| data = response.json() | ||
| sp_data = sp_response.json() | ||
| update_google_sheets(data, sp_data, end_time, name) | ||
|
|
||
| return | ||
| def update_google_sheets(data, sp_data,end_time,name): | ||
| # Convert the timestamp to a datetime object | ||
| SPREADSHEET_ID = '1PQCxTDftKu0nPicnGmLHWJbbkUp0UAFYO1X3STN3izY' # Replace with your Spreadsheet ID | ||
| SHEET_NAME = 'Sheet1' | ||
| last_row = get_last_row(service, SPREADSHEET_ID, SHEET_NAME) | ||
|
|
||
| sheet = service.spreadsheets() | ||
| result = sheet.values().get(spreadsheetId=SPREADSHEET_ID, range='Sheet1!A:A').execute() | ||
| rows = result.get('values', []) | ||
| next_row = len(rows) + 2 # Next row after the last populated cell, plus two for the space | ||
|
|
||
| dt_object = datetime.datetime.utcfromtimestamp(end_time) |
There was a problem hiding this comment.
DRY — HTTP request code is repeated for different queries within the 'fetch_from_newrelic' function, violating the DRY principle.
Suggestion: Extract the HTTP request logic into a separate function that can be reused for different queries.
| rows = result.get('values', []) | ||
| next_row = len(rows) + 2 # Next row after the last populated cell, plus two for the space | ||
|
|
||
| dt_object = datetime.datetime.utcfromtimestamp(end_time) |
There was a problem hiding this comment.
Error Handling — HTTP requests might fail, but there's no specific error handling implemented for failed requests or erroneous data.
Suggestion: Implement error handling for the HTTP request responses and verify their 'status' or 'data' content before processing them.
| :param name: | ||
| :param start_time: | ||
| :param end_time: | ||
| :return: |
There was a problem hiding this comment.
Expressiveness — Use of unexplained string formats and API keys directly in code.
Suggestion: Extract literals like API keys and URL to constants at the beginning of the file or to a config file/environment variables for clarity and easier maintenance.
1 participant
No description provided.