Added console logger and refactored some logs #1388
Conversation
gavin-aguiar requested review from YunchuWang, hallvictoria, pdthummar and vrdmr as code owners 
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@ ## dev #1388 +/- ## ========================================== + Coverage 85.28% 85.47% +0.18% ========================================== Files 35 35 Lines 1971 1969 -2 Branches 371 370 -1 ========================================== + Hits 1681 1683 +2 + Misses 217 213 -4 Partials 73 73
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| Returns: | ||
| T, T : V2 programming model | ||
| T, F : V2 programming model but with invalid file name | ||
| F, T : File path does not exist but filename is valid. |
There was a problem hiding this comment.
There is a special case if someone actually sets it in the app setting, which could be this case and not a V1 programming model. We might want to ensure that in the future releases (TODO: Add another check if the setting was set or not and make the call based on it)
There was a problem hiding this comment.
Added a check if the app setting has been set. If it has, return T F scenario. In dispatcher changed the logs slightly for this case
| debug_logs='Error in load_function. ' | ||
| f'Sys Path: {sys.path}, Sys Module: {sys.modules},' | ||
| 'python-packages Path exists: ' |
There was a problem hiding this comment.
Are you planning to add the exception message for our analysis?
| return func(*args, **kwargs) | ||
| except expt_type as e: | ||
| if debug_logs is not None: | ||
| logger.error(debug_logs) |
There was a problem hiding this comment.
To simplify - You can add the exception to debug_logs itself.
azure_functions_worker/dispatcher.py Outdated
| Exception('Invalid Script file name %s', | ||
| script_file_name))))) | ||
| else: | ||
| logger.error('App setting %s detected but file path %s does ' |
There was a problem hiding this comment.
For this case - how did we detect the app setting explicitly set? Because the current issue is using the default and that'll always return this case.
Also - should this be error_logger?
There was a problem hiding this comment.
This is the False False case, so their script file name was invalid. This means it was changed, not the default
| # Fallback to legacy model | ||
| logger.info("%s does not exist. " | ||
| "Switching to host indexing.", script_file_name) | ||
| if v2_file_exist and file_name_valid: |
There was a problem hiding this comment.
This whole if-else setup needs to be thoroughly tested.
There was a problem hiding this comment.
def _dispatch_response(self, request, function_path, v2_file_exist, file_name_valid): conditions = [ (v2_file_exist, file_name_valid, self._handle_valid_v2), (not v2_file_exist, file_name_valid, self._handle_valid_v1), (v2_file_exist, not file_name_valid, self._handle_invalid_v2), (not v2_file_exist, not file_name_valid, self._handle_invalid_app), ] for condition in conditions: if all(condition[:-1]): return condition[-1](request.request_id, function_path) # Default response or error handling return self._handle_default(request.request_id, function_path) strategy design pattern may be overpowering, but a dispatch map here make code more maintainable.
microsoft-github-policy-service bot added no recent activity and removed no recent activity labels 
| # Handles special case where App Setting is set to incorrect file name | ||
| # for V2 programming model | ||
| if app_setting_set and not path_exists and valid_file_name: | ||
| return True, False |
There was a problem hiding this comment.
move this check at top to cut overhead when failing fast
Description
Added a console logger so that customers can directly see the error message while deploying a function app.
Sample output:
**
**
Fixes #
PR information
Quality of Code and Contribution Guidelines