From 770754e0c241aebf21b149c5346c8ccad274f556 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Mon, 9 Dec 2024 10:46:25 +0000 Subject: [PATCH 01/12] refactor: improved logging arguments added message, status, and type arguments to be logged --- app/controllers/application_controller.rb | 13 +++++++++++-- app/controllers/search_controller.rb | 15 +++++++++++++-- .../application_prometheus_subscriber.rb | 4 ++-- config/initializers/prometheus.rb | 6 +++--- 4 files changed, 29 insertions(+), 9 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 1896f63..6f31196 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true # :nodoc: -class ApplicationController < ActionController::Base +class ApplicationController < ActionController::Base # rubocop:disable Metrics/ClassLength # Prevent CSRF attacks by raising an exception. # For APIs, you may want to use :null_session instead. @@ -38,7 +38,11 @@ def log_request_result rescue_from StandardError do |e| # Instrument ActiveSupport::Notifications for internal errors but only for non-404 errors: unless e.is_a?(ActionController::RoutingError) || e.is_a?(ActionView::MissingTemplate) - ActiveSupport::Notifications.instrument('internal_error.application', exception: e) + instrument_internal_error({ + message: e, + status: e.status || Rack::Utils::SYMBOL_TO_STATUS_CODE[e], + type: e.class.name + }) end # Trigger the appropriate error handling method based on the exception @@ -128,5 +132,10 @@ def detailed_request_log(duration) Rails.logger.info(JSON.generate(log_fields)) end end + + def instrument_internal_error(exc) + ActiveSupport::Notifications.instrument('internal_error.application', exception: exc) + end + # rubocop:enable Metrics/AbcSize, Metrics/MethodLength end diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index 35f4ec6..5adee68 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -56,13 +56,24 @@ def render_error_page(err, message, status, template = 'ppd/error') # link the error to the actual request id otherwise generate one for this error uuid = Thread.current[:request_id] || SecureRandom.uuid + status_code = Rack::Utils::SYMBOL_TO_STATUS_CODE[status] + @message = message # log the error with as much detail as possible in development to aid in resolving the issue - @message = "#{Rack::Utils::SYMBOL_TO_STATUS_CODE[status]} ~ #{err.class.name} error: #{message}" if Rails.env.development? + @message = "#{status_code} ~ #{err.class.name} error: #{message}" if Rails.env.development? # Keep it simple silly in production! - log_error(Rack::Utils::SYMBOL_TO_STATUS_CODE[status], message) + log_error(status_code, message) + + # Trigger metric on internal errors + unless status_code == 404 + instrument_internal_error({ + message: message, + status: status_code, + type: err.class.name + }) + end @error_message = [ diff --git a/app/subscribers/application_prometheus_subscriber.rb b/app/subscribers/application_prometheus_subscriber.rb index 245a6fd..fce5bb2 100644 --- a/app/subscribers/application_prometheus_subscriber.rb +++ b/app/subscribers/application_prometheus_subscriber.rb @@ -5,9 +5,9 @@ class ApplicationPrometheusSubscriber < ActiveSupport::Subscriber attach_to :application def internal_error(event) - message = event.payload[:exception] + error = event.payload[:exception] Prometheus::Client.registry .get(:internal_application_error) - .increment(labels: { message: message.to_s }) + .increment(labels: { result: 'failure', message: "#{error[:type]}: #{error[:message]}", status: error[:status].to_i }) # rubocop:disable Layout/LineLength end end diff --git a/config/initializers/prometheus.rb b/config/initializers/prometheus.rb index 648ed3b..0f55eaf 100644 --- a/config/initializers/prometheus.rb +++ b/config/initializers/prometheus.rb @@ -26,7 +26,7 @@ prometheus.counter( :internal_application_error, docstring: 'Unexpected events and internal error count, labelled by message', - labels: [:message] + labels: %i[message result status] ) # Prometheus gauges @@ -50,5 +50,5 @@ ) # Middleware instrumentation - # This fixes the 0 memory bug by notifying Action Dispatch subscribers on Prometheus initialise - ActiveSupport::Notifications.instrument('process_middleware.action_dispatch') +# This fixes the 0 memory bug by notifying Action Dispatch subscribers on Prometheus initialise +ActiveSupport::Notifications.instrument('process_middleware.action_dispatch') From 885503d13b3b7d0c2dfc9f01e62c24ac6259b321 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Mon, 9 Dec 2024 10:47:15 +0000 Subject: [PATCH 02/12] fix: added catch for missing `SENTRY_API_KEY` env var for docker build --- entrypoint.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/entrypoint.sh b/entrypoint.sh index dc23c17..dbb9fb0 100755 --- a/entrypoint.sh +++ b/entrypoint.sh @@ -8,6 +8,9 @@ mkdir -pm 1777 ./tmp # Set the environment [ -z "$RAILS_ENV" ] && RAILS_ENV=production +# Check for SENTRY_API_KEY variable and report if missing +[ -z "$SENTRY_API_KEY" ] && echo "{\"ts\":\"$(date -u +%FT%T.%3NZ)\",\"level\":\"ERROR\",\"message\":\"SENTRY_API_KEY is required\"}" + # Check for API Service URL variable and log it [ -n "$API_SERVICE_URL" ] && echo "{\"ts\":\"$(date -u +%FT%T.%3NZ)\",\"level\":\"INFO\",\"message\":\"API_SERVICE_URL=${API_SERVICE_URL}\"}" From ff4ae0459f980383b3055008f821bc805ba8d8c5 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Mon, 9 Dec 2024 10:52:01 +0000 Subject: [PATCH 03/12] docs: updated CHANGELOG --- CHANGELOG.md | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1650d47..49cb8b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,14 @@ This app allows the user to explore HMLR price-paid open linked data. ## Changelog +## Unreleased + +- (Jon) Added catch for missing `SENTRY_API_KEY` env var to entrypoint.sh for + docker build +- (Jon) Added `message`, `status`, and `type` arguments to be logged by the + `json_rails_logger` gem for respective error responses +- (Jon) + ## 2.0.0 - 2024-11 - (Bogdan) Updated all gems by regenerating `Gemfile.lock` @@ -13,7 +21,8 @@ This app allows the user to explore HMLR price-paid open linked data. ## 1.8.0 - 2024-10 -- (Dan) Updates ruby version to 2.7.8 and alpine version to 3.16 [GH-253](https://github.com/epimorphics/ppd-explorer/issues/253) +- (Dan) Updates ruby version to 2.7.8 and alpine version to 3.16 + [GH-253](https://github.com/epimorphics/ppd-explorer/issues/253) ## 1.7.11 - 2024-10 From 91ac2ed106b6b2e42e2d609e13e4b0808eb5fd85 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Mon, 16 Dec 2024 16:49:21 +0000 Subject: [PATCH 04/12] style: remove erroneous commented out code --- config/routes.rb | 55 ------------------------------------------------ 1 file changed, 55 deletions(-) diff --git a/config/routes.rb b/config/routes.rb index c8fe04f..abd3cb6 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -8,59 +8,4 @@ resource :ppd_data get '*unmatched_route', to: 'application#render_404' - - # The priority is based upon order of creation: first created -> highest priority. - # See how all your routes lay out with "rake routes". - - # You can have the root of your site routed with "root" - # root 'welcome#index' - - # Example of regular route: - # get 'products/:id' => 'catalog#view' - - # Example of named route that can be invoked with purchase_url(id: product.id) - # get 'products/:id/purchase' => 'catalog#purchase', as: :purchase - - # Example resource route (maps HTTP verbs to controller actions automatically): - # resources :products - - # Example resource route with options: - # resources :products do - # member do - # get 'short' - # post 'toggle' - # end - # - # collection do - # get 'sold' - # end - # end - - # Example resource route with sub-resources: - # resources :products do - # resources :comments, :sales - # resource :seller - # end - - # Example resource route with more complex sub-resources: - # resources :products do - # resources :comments - # resources :sales do - # get 'recent', on: :collection - # end - # end - - # Example resource route with concerns: - # concern :toggleable do - # post 'toggle' - # end - # resources :posts, concerns: :toggleable - # resources :photos, concerns: :toggleable - - # Example resource route within a namespace: - # namespace :admin do - # # Directs /admin/products/* to Admin::ProductsController - # # (app/controllers/admin/products_controller.rb) - # resources :products - # end end From 3f22e3dace9d7f98b824b55eefabd194ff3f83d2 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Mon, 16 Dec 2024 16:50:16 +0000 Subject: [PATCH 05/12] fix: remove error instrumentation from standard error handling --- app/controllers/application_controller.rb | 9 --------- 1 file changed, 9 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 6f31196..fce5c99 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -36,15 +36,6 @@ def log_request_result # or attempt to render a generic error page if no specific error page exists unless Rails.application.config.consider_all_requests_local rescue_from StandardError do |e| - # Instrument ActiveSupport::Notifications for internal errors but only for non-404 errors: - unless e.is_a?(ActionController::RoutingError) || e.is_a?(ActionView::MissingTemplate) - instrument_internal_error({ - message: e, - status: e.status || Rack::Utils::SYMBOL_TO_STATUS_CODE[e], - type: e.class.name - }) - end - # Trigger the appropriate error handling method based on the exception case e.class when ActionController::RoutingError, ActionView::MissingTemplate From de9414fd3dc5f4acb21064b589fbdb3a6dcbdd10 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Mon, 16 Dec 2024 16:51:04 +0000 Subject: [PATCH 06/12] fix: only trigger instrumentation on internal errors --- app/controllers/application_controller.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index fce5c99..c4ebc84 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -56,6 +56,8 @@ def handle_internal_error(exception) render_error(400) else Rails.logger.warn "No explicit error page for exception #{exception} - #{exception.class}" + # Instrument ActiveSupport::Notifications for internal server errors only: + instrument_internal_error(exception) render_error(500) end end From f4382bed1e17af4257fd9d76b14164e7bd80e31e Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Mon, 16 Dec 2024 16:52:24 +0000 Subject: [PATCH 07/12] fix: improve error metrics reporting - ensures that logging always happens with the appropriate severity depending on the exception status - reduces the types of errors that can trigger a an error metric and therefore a notification in slack (e.g. :bad_request is set at WARN level and therefore not alerted on, whereas :internal_server_error is set at ERROR level and does trigger alerts --- app/controllers/application_controller.rb | 25 +++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index c4ebc84..2597695 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -125,10 +125,27 @@ def detailed_request_log(duration) Rails.logger.info(JSON.generate(log_fields)) end end + # rubocop:enable Metrics/AbcSize, Metrics/MethodLength - def instrument_internal_error(exc) - ActiveSupport::Notifications.instrument('internal_error.application', exception: exc) + # Notify subscriber(s) of an internal error event with the payload of the + # exception once done + # @param [exc] exp the exception that caused the error + # @return [ActiveSupport::Notifications::Event] provides an object-oriented + # interface to the event + def instrument_internal_error(exc) # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity + err = { + message: exc&.message || exc, + status: exc&.status || Rack::Utils::SYMBOL_TO_STATUS_CODE[exc] + } + err[:type] = exc.class&.name if exc&.class + err[:cause] = exc&.cause if exc&.cause + err[:backtrace] = exc&.backtrace if exc&.backtrace && Rails.env.development? + # Log the exception to the Rails logger with the appropriate severity + Rails.logger.send(err[:status] < 500 ? :warn : :error, JSON.generate(err)) + # Return unless the status code is 500 or greater to ensure subscribers are NOT notified + return unless err[:status] >= 500 + + # Instrument the internal error event to notify subscribers of the error + ActiveSupport::Notifications.instrument('internal_error.application', exception: err) end - - # rubocop:enable Metrics/AbcSize, Metrics/MethodLength end From c9e934cbb401bb7a4cfa64885111cfd67564929f Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Mon, 16 Dec 2024 17:00:14 +0000 Subject: [PATCH 08/12] build: updates `json_rails_logger` gem package to latest --- Gemfile.lock | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 51155be..cf43990 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -428,10 +428,10 @@ GEM faraday_middleware (~> 1.2.0) json (~> 2.6.1) yajl-ruby (~> 1.4.1) - json_rails_logger (1.0.3) + json_rails_logger (2.0.1) json lograge - railties + railties (~> 7.0) lr_common_styles (2.1.3) bootstrap-sass font-awesome-rails From 9b1b9cef08a85d77ec475a26079c286cac633c51 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Tue, 17 Dec 2024 14:34:06 +0000 Subject: [PATCH 09/12] fix: only instrument internal error on status' 500 or above --- app/controllers/search_controller.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index 5adee68..209e756 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -67,7 +67,7 @@ def render_error_page(err, message, status, template = 'ppd/error') log_error(status_code, message) # Trigger metric on internal errors - unless status_code == 404 + if status_code >= 500 instrument_internal_error({ message: message, status: status_code, @@ -96,5 +96,4 @@ def log_error(status, message) Rails.logger.info(message) end end - end From b004cf103cbe15056b1c10275479658bf31db8ab Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Tue, 17 Dec 2024 14:37:02 +0000 Subject: [PATCH 10/12] fix: resolve missing error templates update to use `Rails.public_path` alongside ending the path with `.html` to be sure --- app/controllers/application_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 2597695..12f3a8f 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -90,7 +90,7 @@ def render_error(status) def render_html_error_page(status) render(layout: true, - file: Rails.root.join('public', 'landing', status.to_s), + file: Rails.public_path + "landing/#{status}.html", status: status) end From 4957ec27b3cd0bf3d6089229356bc28579daf6d1 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Tue, 17 Dec 2024 14:49:35 +0000 Subject: [PATCH 11/12] docs: updated CHANGELOG --- CHANGELOG.md | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 49cb8b8..fddb31f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,13 +4,21 @@ This app allows the user to explore HMLR price-paid open linked data. ## Changelog -## Unreleased - -- (Jon) Added catch for missing `SENTRY_API_KEY` env var to entrypoint.sh for +## 2.0.1 - 2024-12 + +- (Jon) Updated the error template path to use `Rails.public_path` as well as + contain the `html` extension to ensure the correct template is rendered +- (Jon) Improves error metrics reporting to ensure that logging always happens + with the appropriate severity depending on the exception status while reducing + the types of errors that can trigger a an error metric and therefore a + notification in slack + [GH-149](https://github.com/epimorphics/hmlr-linked-data/issues/149) +- (Jon) Added catch for missing `SENTRY_API_KEY` env var to `entrypoint.sh` for docker build +- (Jon) Updated target status level to trigger internal error metrics to 500 + status codes only in the `search_controller` - (Jon) Added `message`, `status`, and `type` arguments to be logged by the - `json_rails_logger` gem for respective error responses -- (Jon) + `json_rails_logger` gem for respective error responses in the `search_controller` ## 2.0.0 - 2024-11 From 98d903ce2681c9632d6a855b4998707088d002fc Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Tue, 17 Dec 2024 14:50:05 +0000 Subject: [PATCH 12/12] build: updated patch version cadence --- app/lib/version.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/lib/version.rb b/app/lib/version.rb index da348ad..4d205e6 100644 --- a/app/lib/version.rb +++ b/app/lib/version.rb @@ -3,7 +3,7 @@ module Version MAJOR = 2 MINOR = 0 - PATCH = 0 + PATCH = 1 SUFFIX = nil VERSION = "#{MAJOR}.#{MINOR}.#{PATCH}#{SUFFIX && ".#{SUFFIX}"}" end