From ebf51ae1591d9f740bb54dbf274970e2917e0afa Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Tue, 30 Apr 2024 17:26:58 +0100 Subject: [PATCH 01/35] docs: updated CHANGELOG refactored v1.7.4 tag date to reflect latest changes --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cbbbd85a..eacb1bca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## unreleased -## 1.7.4 - 2024-04-19 +## 1.7.4 - 2024-05-01 - (Jon) Updated print presenter to use [`.try(:first)`](https://apidock.com/rails/Object/try) which resolves by From bd65daee7441e05d0d0940f6b396a5a112a93735 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Wed, 9 Oct 2024 10:59:57 +0100 Subject: [PATCH 02/35] refactor: renamed `render_error` method to `handle_error` also updated method call in exceptions initialiser --- app/controllers/exceptions_controller.rb | 2 +- config/initializers/exceptions.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/exceptions_controller.rb b/app/controllers/exceptions_controller.rb index 11c20945..25ab4f99 100644 --- a/app/controllers/exceptions_controller.rb +++ b/app/controllers/exceptions_controller.rb @@ -4,7 +4,7 @@ class ExceptionsController < ApplicationController layout 'application' - def render_error + def handle_error env = request.env exception = env['action_dispatch.exception'] status_code = ActionDispatch::ExceptionWrapper.new(env, exception).status_code diff --git a/config/initializers/exceptions.rb b/config/initializers/exceptions.rb index b0de5388..548b982b 100644 --- a/config/initializers/exceptions.rb +++ b/config/initializers/exceptions.rb @@ -3,5 +3,5 @@ # Custom error handling via a Rack middleware action Rails.application.config.exceptions_app = lambda do |env| - ExceptionsController.action(:render_error).call(env) + ExceptionsController.action(:handle_error).call(env) end From 5322aa4e9681c706115bf801fbdb0b4eac5af3a8 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Wed, 9 Oct 2024 11:02:07 +0100 Subject: [PATCH 03/35] refactor: adjusted internal error instrumentation Set the Internal Error Instrumentation to an `unless` statement to ensure the application does not report internal errors to the Prometheus metrics when the error is a 404 thereby reducing the noise in the Slack alerts channel --- app/controllers/exceptions_controller.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/app/controllers/exceptions_controller.rb b/app/controllers/exceptions_controller.rb index 25ab4f99..538649dc 100644 --- a/app/controllers/exceptions_controller.rb +++ b/app/controllers/exceptions_controller.rb @@ -10,8 +10,9 @@ def handle_error status_code = ActionDispatch::ExceptionWrapper.new(env, exception).status_code sentry_code = maybe_report_to_sentry(exception, status_code) - # add the exception to the prometheus metrics - instrument_internal_error(exception) + + # add the exception to the prometheus metrics but only on errors that are 404 + instrument_internal_error(exception) unless status_code == 404 render :error_page, locals: { status: status_code, sentry_code: sentry_code }, @@ -22,7 +23,7 @@ def handle_error private def maybe_report_to_sentry(exception, status_code) - return nil if Rails.env.development? # Why are we reporting to Senty in dev? + return nil if Rails.env.development? # Why are we reporting to Sentry in dev? return nil unless status_code >= 500 sevent = Sentry.capture_exception(exception) From 9b9afbeda1a29fd41426af7b0e680da04a79cf56 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Wed, 9 Oct 2024 11:04:13 +0100 Subject: [PATCH 04/35] refactor: adjusted application error reporting Split the error logging into it's own method as well as adjusted the logged message to be either the response message or the response status --- app/controllers/application_controller.rb | 24 +++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 34285f77..dfde505d 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -39,7 +39,7 @@ def log_request_result private - # rubocop:disable Metrics/AbcSize + # rubocop:disable Metrics/AbcSize, Metrics/MethodLength def detailed_request_log(duration) env = request.env @@ -54,20 +54,28 @@ def detailed_request_log(duration) body: request.body.gets&.gsub("\n", '\n'), method: request.method, status: response.status, - message: Rack::Utils::HTTP_STATUS_CODES[response.status] + message: response.message || Rack::Utils::HTTP_STATUS_CODES[response.status] } - case response.status - when 500..599 + if (500..599).include?(Rack::Utils::SYMBOL_TO_STATUS_CODE[response.status]) log_fields[:message] = env['action_dispatch.exception'].to_s - Rails.logger.error(JSON.generate(log_fields)) + end + + log_response(response.status, log_fields) + end + # rubocop:enable Metrics/AbcSize, Metrics/MethodLength + + # Log the error with the appropriate log level based on the status code + def log_response(status, error_log) + case status + when 500..599 + Rails.logger.error(JSON.generate(error_log)) when 400..499 - Rails.logger.warn(JSON.generate(log_fields)) + Rails.logger.warn(JSON.generate(error_log)) else - Rails.logger.info(JSON.generate(log_fields)) + Rails.logger.info(JSON.generate(error_log)) end end - # rubocop:enable Metrics/AbcSize # Notify subscriber(s) of an internal error event with the payload of the # exception once done From a8804e36f1ab1528c658f46c0f3f60e6c6cfca1e Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Wed, 9 Oct 2024 11:08:07 +0100 Subject: [PATCH 05/35] build: updated the version patch cadence from 1.7.5 ~> 1.7.6 --- 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 011f92f0..2e2f2907 100644 --- a/app/lib/version.rb +++ b/app/lib/version.rb @@ -3,7 +3,7 @@ module Version MAJOR = 1 MINOR = 7 - PATCH = 5 + PATCH = 6 SUFFIX = nil VERSION = "#{MAJOR}.#{MINOR}.#{PATCH}#{SUFFIX && ".#{SUFFIX}"}" end From 8341e4ad1656288313c299d677a82753fda0d20e Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Wed, 9 Oct 2024 11:08:23 +0100 Subject: [PATCH 06/35] docs: Updated CHANGELOG --- CHANGELOG.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3283a9dc..df43f655 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,15 @@ # Changes to the UKHPI app by version and date +## 1.7.6 - 2024-10 + +- (Jon) Split the error logging into it's own method as well as adjusted the + logged message to be either the response message or the response status +- (Jon) Renamed `render_error` method to `handle_error` +- (Jon) Set the Internal Error Instrumentation to an `unless` statement to + ensure the application does not report internal errors to the Prometheus + metrics when the error is a 404 thereby reducing the noise in the Slack alerts + channel + ## 1.7.5 - 2024-09 - (Jon) Created a `local` makefile target to allow for local development without From 6c13e783bc9321d95a3ef53130323994299f33a9 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Tue, 12 Nov 2024 16:09:17 +0000 Subject: [PATCH 07/35] build: implement Rails Panel - Chrome extension for debugging rails applications in the browser: https://chromewebstore.google.com/detail/rails-panel/gjpfobpafnhjhbajcjgccbbdofdckggg?pli=1 --- Gemfile | 4 ++++ Gemfile.lock | 6 ++++++ 2 files changed, 10 insertions(+) diff --git a/Gemfile b/Gemfile index 1ea5d20f..3aa96454 100644 --- a/Gemfile +++ b/Gemfile @@ -49,6 +49,10 @@ group :development, :test do end group :development do + # Devtools panel for Rails development + # https://chromewebstore.google.com/detail/rails-panel/gjpfobpafnhjhbajcjgccbbdofdckggg?pli=1 + gem 'meta_request' + # Access an IRB console on exception pages or by using <%= console %> in views gem 'web-console' diff --git a/Gemfile.lock b/Gemfile.lock index 25745edf..d9334b94 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -210,6 +210,9 @@ GEM net-smtp marcel (1.0.4) matrix (0.4.2) + meta_request (0.8.4) + rack-contrib (>= 1.1, < 3) + railties (>= 3.0.0, < 8) method_source (1.1.0) mini_mime (1.1.5) minitest (5.25.1) @@ -266,6 +269,8 @@ GEM puma (>= 6.0) racc (1.8.1) rack (3.1.8) + rack-contrib (2.5.0) + rack (< 4) rack-proxy (0.7.7) rack rack-session (2.0.0) @@ -475,6 +480,7 @@ DEPENDENCIES json_expressions json_rails_logger (~> 1.0.0)! m + meta_request minitest-rails minitest-reporters mocha From af272b37861ee5596e45bd1bc65339c09b6751f1 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Tue, 12 Nov 2024 16:10:19 +0000 Subject: [PATCH 08/35] fix: refactor time taken to be milliseconds not microseconds --- app/models/latest_values_command.rb | 4 ++-- app/services/query_command.rb | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/models/latest_values_command.rb b/app/models/latest_values_command.rb index aaa43dad..35f50ec1 100644 --- a/app/models/latest_values_command.rb +++ b/app/models/latest_values_command.rb @@ -50,8 +50,8 @@ def run_query(hpi) # rubocop:disable Metrics/AbcSize Rails.logger.error { "API query failed with: #{e.inspect}" } success = false end - time_taken = (Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) - start) - Rails.logger.info { format("API query '#{query.to_json}' completed in %.0f ฮผs\n", time_taken) } + time_taken = (Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) - start) / 1000.0 + Rails.logger.info { format("API query '#{query.to_json}' completed in %.0f ms\n", time_taken) } success end diff --git a/app/services/query_command.rb b/app/services/query_command.rb index ee79337d..7dded0ec 100644 --- a/app/services/query_command.rb +++ b/app/services/query_command.rb @@ -15,8 +15,8 @@ def initialize(user_selections) # @param service Optional API service end-point to use. Defaults to the UKHPI # API service endpoint def perform_query(service = nil) - time_taken = execute_query(service, query) - Rails.logger.info(format("API roundtrip took %.0f ฮผs\n", time_taken)) + time_taken = execute_query(service, query) / 1000.0 + Rails.logger.info(format("API roundtrip took %.0f ms\n", time_taken)) end # @return True if this a query execution command From 5dc661b1c6f568b793c3c2020486e96dca9b4e21 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Tue, 12 Nov 2024 17:16:43 +0000 Subject: [PATCH 09/35] build: add puma stats to webpack partial render also includes minor empty line updates --- app/views/layouts/webpack_application.html.haml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/views/layouts/webpack_application.html.haml b/app/views/layouts/webpack_application.html.haml index 3e5ef16c..3bb8f236 100644 --- a/app/views/layouts/webpack_application.html.haml +++ b/app/views/layouts/webpack_application.html.haml @@ -6,6 +6,7 @@ %meta{ name: 'viewport', content: 'width=device-width, initial-scale=1, shrink-to-fit=no' } %title = (yield(:title) + " - " unless yield(:title).blank?).to_s + I18n.t('common.header.app_title') + :javascript document.querySelector('html').classList.add('js'); window.ukhpi = window.ukhpi || {}; @@ -16,7 +17,8 @@ - if Rails.env.production? = render partial: 'common/google-analytics' = javascript_include_tag 'cookie', defer: true - = csrf_meta_tags + + = csrf_meta_tags = stylesheet_link_tag 'application', media: 'all' = render partial: 'common/favicons' @@ -46,3 +48,4 @@ = yield = render partial: 'common/footer' + = render partial: 'common/puma_stats' if Rails.env.development? From ba4bee2df45822c2446176a0d29470490e42ec0a Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Tue, 12 Nov 2024 17:33:57 +0000 Subject: [PATCH 10/35] build: implement `puma-metrics` plugin post package updates also cleaned up the outdated comments for the plugin --- config/puma.rb | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/config/puma.rb b/config/puma.rb index d8d43964..766ebdf9 100644 --- a/config/puma.rb +++ b/config/puma.rb @@ -14,10 +14,6 @@ # default is 3000. port ENV.fetch('PORT', 3000) -# Specifies the `metrics_port` that Puma will listen on to export metrics; -# default is 9393. -# metrics_port = ENV.fetch('METRICS_PORT', 9393) - # Specifies the `environment` that Puma will run in. # environment ENV.fetch('RAILS_ENV', 'development') @@ -53,12 +49,15 @@ # Allow puma to be restarted by `rails restart` command. plugin :tmp_restart -# Uncomment the following line once ruby is updated to 2.7 or greater to allow -# the use of the puma-metrics plugin as we're using puma 6.0.0 or greater # Additional metrics from the Puma server to be exposed in the /metrics endpoint -# plugin :metrics +plugin :metrics + +# Specifies the `metrics_port` that Puma will listen on to export metrics; +# default is 9393. +metrics_port = ENV.fetch('METRICS_PORT', 9393) + # Bind the metric server to "url". "tcp://" is the only accepted protocol. -# metrics_url "tcp://0.0.0.0:#{metrics_port}" if Rails.env.development? +metrics_url "tcp://0.0.0.0:#{metrics_port}" if Rails.env.development? # Use a custom log formatter to emit Puma log messages in a JSON format log_formatter do |str| From 1e30fff080a0e843a76eeaffcac7bf135499dc50 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Wed, 13 Nov 2024 10:13:14 +0000 Subject: [PATCH 11/35] fix: moved `instrument_internal_error` method to ensure the metric is only triggered on errors with a 500 status or greater --- app/controllers/exceptions_controller.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/exceptions_controller.rb b/app/controllers/exceptions_controller.rb index 538649dc..d12226f0 100644 --- a/app/controllers/exceptions_controller.rb +++ b/app/controllers/exceptions_controller.rb @@ -11,9 +11,6 @@ def handle_error sentry_code = maybe_report_to_sentry(exception, status_code) - # add the exception to the prometheus metrics but only on errors that are 404 - instrument_internal_error(exception) unless status_code == 404 - render :error_page, locals: { status: status_code, sentry_code: sentry_code }, layout: true, @@ -26,6 +23,9 @@ def maybe_report_to_sentry(exception, status_code) return nil if Rails.env.development? # Why are we reporting to Sentry in dev? return nil unless status_code >= 500 + # Trigger error metrics on 5xx errors only to reduce slack noise + instrument_internal_error(exception) + sevent = Sentry.capture_exception(exception) sevent&.event_id end From 183a577afd4bf30aa516f321f4dff56b12cae7b1 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Wed, 13 Nov 2024 10:54:07 +0000 Subject: [PATCH 12/35] style: fixing comments --- app/models/concerns/user_choices.rb | 2 +- app/models/user_selections.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/concerns/user_choices.rb b/app/models/concerns/user_choices.rb index 7130e764..bfe77ca8 100644 --- a/app/models/concerns/user_choices.rb +++ b/app/models/concerns/user_choices.rb @@ -55,7 +55,7 @@ def alternative_key(key) # Recognise a date. Accepts ISO-8601 strings or Date objects. # Dates that match YYYY-MM will be transformed to YYYY-MM-01. - # @param date Either an ISO0-8601 date string, or a date object + # @param date Either an ISO-8601 date string, or a date object def parse_date(date) if date.is_a?(Date) date diff --git a/app/models/user_selections.rb b/app/models/user_selections.rb index e8fc2af1..53300b5f 100644 --- a/app/models/user_selections.rb +++ b/app/models/user_selections.rb @@ -8,7 +8,7 @@ # standard set of statistics is presented if there is no information in the # user parameters yet. This functionality combines the previous # `models/UserPreferences` and `presenters/Aspects` -class UserSelections +class UserSelections # rubocop:disable Metrics/ClassLength include UserChoices include UserSelectionValidations include UserLanguage From d180a211ff7a05b7d6068bc22499eb662d3187eb Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Wed, 13 Nov 2024 11:19:55 +0000 Subject: [PATCH 13/35] feat: improved logging now using hash approach to ensure specific logging fields are included in messages --- app/controllers/application_controller.rb | 21 +++--- app/models/latest_values_command.rb | 82 +++++++++++++++++------ app/services/query_command.rb | 18 ++++- 3 files changed, 88 insertions(+), 33 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index dfde505d..21334501 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -39,31 +39,36 @@ def log_request_result private - # rubocop:disable Metrics/AbcSize, Metrics/MethodLength + # rubocop:disable Metrics/AbcSize, Metrics/MethodLength, Layout/LineLength def detailed_request_log(duration) env = request.env log_fields = { + accept: env['HTTP_ACCEPT'], + body: request.body.gets&.gsub("\n", '\n'), duration: duration, - request_id: env['X_REQUEST_ID'], forwarded_for: env['X_FORWARDED_FOR'], + message: response.message || Rack::Utils::HTTP_STATUS_CODES[response.status], path: env['REQUEST_PATH'], query_string: env['QUERY_STRING'], - user_agent: env['HTTP_USER_AGENT'], - accept: env['HTTP_ACCEPT'], - body: request.body.gets&.gsub("\n", '\n'), + request_id: env['X_REQUEST_ID'], + request_uri: env['REQUEST_URI'], method: request.method, - status: response.status, - message: response.message || Rack::Utils::HTTP_STATUS_CODES[response.status] + status: response.status } + if env['HTTP_USER_AGENT'] && Rails.env.production? + log_fields[:user_agent] = env['HTTP_USER_AGENT'] + end + if (500..599).include?(Rack::Utils::SYMBOL_TO_STATUS_CODE[response.status]) log_fields[:message] = env['action_dispatch.exception'].to_s + log_fields[:backtrace] = env['action_dispatch.backtrace'].join("\n") unless Rails.env.production? end log_response(response.status, log_fields) end - # rubocop:enable Metrics/AbcSize, Metrics/MethodLength + # rubocop:enable Metrics/AbcSize, Metrics/MethodLength, Layout/LineLength # Log the error with the appropriate log level based on the status code def log_response(status, error_log) diff --git a/app/models/latest_values_command.rb b/app/models/latest_values_command.rb index 35f50ec1..e716f253 100644 --- a/app/models/latest_values_command.rb +++ b/app/models/latest_values_command.rb @@ -7,6 +7,11 @@ class LatestValuesCommand attr_reader :results def perform_query(service = nil) + log_fields = {} + log_fields[:message] = 'Received Data Services API query' + log_fields[:request_status] = 'received' + + Rails.logger.info(JSON.generate(log_fields)) hpi = service_api(service) (hpi && run_query(hpi)) || no_service @@ -14,26 +19,45 @@ def perform_query(service = nil) private - def service_api(service) # rubocop:disable Metrics/AbcSize - service || dataset(:ukhpi) - rescue Faraday::ConnectionFailed => e - Rails.logger.error { 'Failed to connect to UK HPI ' } - Rails.logger.error { "Status: #{e.status}, body: '#{e.message}'" } - Rails.logger.error { e } - nil - rescue DataServicesApi::ServiceException => e - Rails.logger.error { 'Failed to get response from UK HPI service' } - Rails.logger.error { "Status: #{e.status}, body: '#{e.service_message}'" } - nil - rescue RuntimeError => e - Rails.logger.error { "Runtime error #{e.inspect}" } - Rails.logger.error { e.class } - Rails.logger.error { e.backtrace.join("\n") } - Rails.logger.error { "Caused by: #{e.cause}" } if e.cause - nil + def service_api(service) # rubocop:disable Metrics/AbcSize, Metrics/MethodLength, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity + log_fields = {} + service = nil if service.blank? + begin + service || dataset(:ukhpi) + log_fields[:message] = 'Connected to UK HPI service' + log_type = 'info' + rescue Faraday::ConnectionFailed => e + log_fields[:message] = 'Failed to connect to UK HPI service' + log_fields[:status] = e.status + log_fields[:body] = e.message + log_fields[:backtrace] = e&.backtrace&.join("\n") if Rails.env.development? + log_fields[:request_status] = 'error' + + service = nil + rescue DataServicesApi::ServiceException => e + log_fields[:message] = 'Failed to get response from UK HPI service' + log_fields[:status] = e.status + log_fields[:body] = e.service_message + log_fields[:request_status] = 'error' + + service = nil + rescue RuntimeError => e + log_fields[:message] = "Runtime error #{e.inspect}" + log_fields[:status] = e.status + log_fields[:body] = "Caused by: #{e.cause} in " if e.cause + log_fields[:body] += e.class + log_fields[:backtrace] = e&.backtrace&.join("\n") if Rails.env.development? + log_fields[:request_status] = 'error' + + service = nil + end + Rails.logger.send(log_type) { log_fields } + service end - def run_query(hpi) # rubocop:disable Metrics/AbcSize + def run_query(hpi) # rubocop:disable Metrics/AbcSize, Metrics/MethodLength + log_fields = {} + success = true query = add_date_range_constraint(base_query) query = add_location_constraint(query) @@ -43,15 +67,29 @@ def run_query(hpi) # rubocop:disable Metrics/AbcSize start = Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) begin @results = hpi.query(query) + time_taken = (Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) - start) / 1000 + message = format("Processing Data Services API query: '#{query.to_json}' in %.0fms", time_taken) + log_fields[:duration] = time_taken + log_fields[:message] = message + log_fields[:request_status] = 'processing' + log_type = 'info' rescue NoMethodError => e - Rails.logger.debug { "Application failed with: NoMethodError #{e.inspect}" } + log_fields[:message] = "Application failed with: NoMethodError: #{e.inspect}" + log_fields[:request_status] = 'error' + log_type = 'error' + success = false + rescue ArgumentError => e + log_fields[:message] = "Data Services API query failed with: ArgumentError: #{e.inspect}" + log_fields[:request_status] = 'error' + log_type = 'error' success = false rescue RuntimeError => e - Rails.logger.error { "API query failed with: #{e.inspect}" } + log_fields[:message] = "Data Services API query failed with: #{e.inspect}" + log_fields[:request_status] = 'error' + log_type = 'error' success = false end - time_taken = (Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) - start) / 1000.0 - Rails.logger.info { format("API query '#{query.to_json}' completed in %.0f ms\n", time_taken) } + Rails.logger.send(log_type) { JSON.generate(log_fields) } success end diff --git a/app/services/query_command.rb b/app/services/query_command.rb index 7dded0ec..07143e03 100644 --- a/app/services/query_command.rb +++ b/app/services/query_command.rb @@ -15,8 +15,12 @@ def initialize(user_selections) # @param service Optional API service end-point to use. Defaults to the UKHPI # API service endpoint def perform_query(service = nil) - time_taken = execute_query(service, query) / 1000.0 - Rails.logger.info(format("API roundtrip took %.0f ms\n", time_taken)) + log_fields = {} + time_taken = execute_query(service, query) + log_fields[:message] = format("Completed Data Services API roundtrip took %.0fms\n", time_taken) + log_fields[:request_status] = 'completed' + log_fields[:duration] = time_taken + Rails.logger.info(log_fields) end # @return True if this a query execution command @@ -33,9 +37,13 @@ def explain_query_command? # Construct the DsAPI query that matches the given user constraints def build_query + log_fields = {} + log_fields[:message] = 'Received Data Services API query' + log_fields[:request_status] = 'received' query = add_date_range_constraint(base_query) query1 = add_location_constraint(query) add_sort(query1) + Rails.logger.info(log_fields) end def api_service(service) @@ -48,9 +56,13 @@ def default_service # Run the given query, stash the results, and return time taken in microseconds. def execute_query(service, query) + log_fields = {} + log_fields[:message] = 'Processing Data Services API query' + log_fields[:request_status] = 'processing' start = Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) @results = api_service(service).query(query) - (Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) - start) + Rails.logger.info(log_fields) + (Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) - start) / 1000 end def add_date_range_constraint(query) From 46825cc4f2565c51e6b8b8ef1bd3b2232626be52 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Wed, 13 Nov 2024 11:20:14 +0000 Subject: [PATCH 14/35] style: rubocop comment updates --- app/controllers/browse_controller.rb | 2 +- app/controllers/compare_controller.rb | 2 +- test/models/user_selections_test.rb | 72 ++++++++++++++++++++------- test/services/query_command_test.rb | 4 +- 4 files changed, 59 insertions(+), 21 deletions(-) diff --git a/app/controllers/browse_controller.rb b/app/controllers/browse_controller.rb index 8b7f2a3e..bd76d4de 100644 --- a/app/controllers/browse_controller.rb +++ b/app/controllers/browse_controller.rb @@ -3,7 +3,7 @@ # Controller for the main user experience of browsing the UKHPI statistics. # Usually the primary interaction will be via JavaScript and XHR, but we also # support non-JS access by setting browse preferences in the `edit` action. -class BrowseController < ApplicationController +class BrowseController < ApplicationController # rubocop:disable Metrics/ClassLength layout 'webpack_application' def show diff --git a/app/controllers/compare_controller.rb b/app/controllers/compare_controller.rb index 4998baf9..a66da0ca 100644 --- a/app/controllers/compare_controller.rb +++ b/app/controllers/compare_controller.rb @@ -31,7 +31,7 @@ def render_print render 'compare/print', layout: 'print' end - def perform_query(user_compare_selections) + def perform_query(user_compare_selections) # rubocop:disable Metrics/MethodLength query_results = {} base_selection = UserSelections.new( __safe_params: { diff --git a/test/models/user_selections_test.rb b/test/models/user_selections_test.rb index d305c425..0477cced 100644 --- a/test/models/user_selections_test.rb +++ b/test/models/user_selections_test.rb @@ -77,7 +77,7 @@ class UserSelectionsTest < ActiveSupport::TestCase end end - describe '#with' do + describe '#with_the_new_singular_value' do it 'should create a new user preferences with the new singular value' do selections0 = user_selections('location' => 'test-region-0') _(selections0.selected_location).must_equal 'test-region-0' @@ -93,7 +93,9 @@ class UserSelectionsTest < ActiveSupport::TestCase _(selections2.selected_location).must_equal 'test-region-2' _(selections2.from_date).must_equal Date.new(2017, 3, 25) end + end + describe '#with_the_additional_array_value' do it 'should create a new user preferences with an additional array value' do selections0 = user_selections('in' => ['averagePrice']) _(selections0.selected_indicators).must_equal ['averagePrice'] @@ -112,7 +114,9 @@ class UserSelectionsTest < ActiveSupport::TestCase _(selections2.selected_indicators).must_include 'percentageMonthlyChange' _(selections2.selected_indicators).must_include 'percentageAnnualChange' end + end + describe '#with_array_valued_params' do it 'should not create duplicate values in array-valued params' do selections0 = user_selections('in' => ['averagePrice']) selections1 = selections0.with('in', 'averagePrice') @@ -121,7 +125,7 @@ class UserSelectionsTest < ActiveSupport::TestCase end end - describe '#without' do + describe '#without_the_given_key_for_singlular_values' do it 'should create a new user preferences value without the given key for singlular values' do selections0 = user_selections('location' => 'test-region-0') _(selections0.selected_location).must_equal 'test-region-0' @@ -131,7 +135,9 @@ class UserSelectionsTest < ActiveSupport::TestCase assert selections0.params.key?('location') assert_not selections1.params.key?('location') end + end + describe '#without_the_given_value_for_array_values' do it 'should create a new user preferences value without the given value for array values' do selections0 = user_selections('in' => %w[averagePrice percentageMonthlyChange]) _(selections0.selected_indicators.length).must_equal 2 @@ -158,19 +164,34 @@ class UserSelectionsTest < ActiveSupport::TestCase end end - describe '#valid?' do + describe '#empty_model_is_valid?' do it 'should report the empty model is valid' do selections = user_selections({}) _(selections.valid?).must_equal(true) end + end + + describe '#formatted_date_is_valid?' do + it 'should allow a YYYY-MM date as valid' do + selections = user_selections( + 'from' => '2017-01' + ) + _(selections.valid?).must_equal(true) + _(selections.errors).must_be_empty + end + end + + describe '#from_date_is_valid?' do it 'should report that a correctly formatted from date is valid' do selections = user_selections( 'from' => '2017-01-01' ) _(selections.valid?).must_equal(true) end + end + describe '#from_date_is_invalid?' do it 'should report that an incorrectly formatted from date is invalid' do selections = user_selections( 'from' => '2017-0' @@ -178,15 +199,10 @@ class UserSelectionsTest < ActiveSupport::TestCase _(selections.valid?).must_equal(false) _(selections.errors).must_include('incorrect or missing "from" date') end + end - it 'should allow a YYYY-MM date as valid' do - selections = user_selections( - 'from' => '2017-01' - ) - _(selections.valid?).must_equal(true) - _(selections.errors).must_be_empty - end + describe '#to_date_is_invalid?' do it 'should report that an incorrectly formatted to date is invalid' do selections = user_selections( 'to' => '2017-0' @@ -194,14 +210,18 @@ class UserSelectionsTest < ActiveSupport::TestCase _(selections.valid?).must_equal(false) _(selections.errors).must_include('incorrect or missing "to" date') end + end + describe '#location_is_valid?' do it 'should report that a correct location URI is valid' do selections = user_selections( location: 'http://landregistry.data.gov.uk/id/region/united-kingdom' ) _(selections.valid?).must_equal(true) end + end + describe '#location_is_invalid?' do it 'should report that an incorrect location URI is invalid' do selections = user_selections( location: 'http://landregistry.data.gov.uk/id/region/' @@ -209,35 +229,53 @@ class UserSelectionsTest < ActiveSupport::TestCase _(selections.valid?).must_equal(false) _(selections.errors).must_include('unrecognised location') end + end + describe '#indicator_is_valid?' do it 'should report that a correctly stated indicator is valid' do selections = user_selections('in' => %w[pac pmc]) _(selections.valid?).must_equal(true) end + end + describe '#indicator_is_invalid?' do it 'should report that an incorrectly stated indicator is not valid' do selections = user_selections('in' => %w[pmc percentageMonthlyWombles]) _(selections.valid?).must_equal(false) end + end + describe '#statistic_is_valid?' do it 'should report that a correctly stated statistic is valid' do selections = user_selections('st' => %w[det sem]) _(selections.valid?).must_equal(true) end + end + describe '#statistic_is_invalid?' do it 'should report that an incorrectly stated statistic is not valid' do selections = user_selections('st' => %w[det percentageMonthlyWombles]) _(selections.valid?).must_equal(false) end end - describe 'language handling' do + describe '#default_language_selected' do it 'should return English as the default' do selections = user_selections({}) assert selections.english? assert_not selections.welsh? end + end + describe '#default_language_params' do + it 'should return English when that language is selected' do + selections = user_selections('lang' => 'en') + assert selections.english? + assert_not selections.welsh? + end + end + + describe '#alternative_language_params' do it 'should return Welsh when that language is selected' do selections = user_selections('lang' => 'cy') current_locale = I18n.locale @@ -248,19 +286,17 @@ class UserSelectionsTest < ActiveSupport::TestCase ensure I18n.locale = current_locale end + end - it 'should return English when that language is selected' do - selections = user_selections('lang' => 'en') - assert selections.english? - assert_not selections.welsh? - end - + describe '#ignore_other_languages' do it 'should ignore other languages' do selections = user_selections('lang' => 'fr') assert selections.english? assert_not selections.welsh? end + end + describe '#alternative_language_params_switch' do it 'should generate the correct options to switch to Welsh language' do selections = user_selections( 'from' => '2017-01' @@ -270,7 +306,9 @@ class UserSelectionsTest < ActiveSupport::TestCase _(alt_params.params['from']).must_equal('2017-01') _(alt_params.params['lang']).must_equal('cy') end + end + describe '#default_language_params_switch' do it 'should generate the correct options to switch to English language' do selections = user_selections( 'from' => '2017-01', diff --git a/test/services/query_command_test.rb b/test/services/query_command_test.rb index b3c53a8c..9f83465a 100644 --- a/test/services/query_command_test.rb +++ b/test/services/query_command_test.rb @@ -2,7 +2,7 @@ require 'test_helper' -class MockService +class MockService # :nodoc: attr_reader :captured def query(query) @@ -10,7 +10,7 @@ def query(query) end end -def validate_json(json) +def validate_json(json) # rubocop:disable Metrics/MethodLength _(json).must_match_json_expression( '@and': [ From 07db73abd4f470910da6eeec31393bef275d2b7a Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Mon, 9 Dec 2024 11:39:26 +0000 Subject: [PATCH 15/35] refactor: update logging from makefile targets --- Makefile | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/Makefile b/Makefile index 3934ec9a..8b64b65b 100644 --- a/Makefile +++ b/Makefile @@ -37,8 +37,11 @@ ${GITHUB_TOKEN}: @echo ${PAT} > ${GITHUB_TOKEN} assets: + @echo "Installing bundler packages ..." @./bin/bundle install + @echo "Installing yarn packages ..." @yarn install + @echo "Cleaning and precompiling static assets ..." @./bin/rails assets:clean assets:precompile auth: ${GITHUB_TOKEN} ${BUNDLE_CFG} @@ -68,14 +71,6 @@ image: auth lint: assets @./bin/bundle exec rubocop -local: - @echo "Installing bundler packages ..." - @./bin/bundle install - @echo "Installing yarn packages ..." - @yarn install - @echo "Starting local server ..." - @./bin/rails server -p ${PORT} - publish: image @echo Publishing image: ${REPO}:${TAG} ... @docker tag ${NAME}:${TAG} ${REPO}:${TAG} 2>&1 @@ -89,8 +84,12 @@ run: start @if docker network inspect dnet > /dev/null 2>&1; then echo "Using docker network dnet"; else echo "Create docker network dnet"; docker network create dnet; sleep 2; fi @docker run -p ${PORT}:3000 -e API_SERVICE_URL=${API_SERVICE_URL} --network dnet --rm --name ${SHORTNAME} ${NAME}:${TAG} -server: assets start +secret: + @echo "Creating secret ..." @export SECRET_KEY_BASE=$(./bin/rails secret) + +server: + @echo "Starting local server ..." @API_SERVICE_URL=${API_SERVICE_URL} ./bin/rails server -p ${PORT} start: From fe1b05c60b8e215dd7ff146d1708288ed1e18052 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Tue, 7 Jan 2025 15:20:46 +0000 Subject: [PATCH 16/35] build: remove duplicated gem entry --- Gemfile | 3 --- 1 file changed, 3 deletions(-) diff --git a/Gemfile b/Gemfile index aa0090fa..d62d84ee 100644 --- a/Gemfile +++ b/Gemfile @@ -52,11 +52,8 @@ group :development do # Devtools panel for Rails development # https://chromewebstore.google.com/detail/rails-panel/gjpfobpafnhjhbajcjgccbbdofdckggg?pli=1 gem 'meta_request' - # Access an IRB console on exception pages or by using <%= console %> in views gem 'web-console' - # Rails Panel is a Chrome extension for Rails development that will end your tailing of development.log. - gem 'meta_request' # Spring speeds up development by keeping your application running in the background. Read more: https://github.com/rails/spring gem 'spring' end From 59b75f0998d175c559d03b2fd0cb47bc46d6e858 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Fri, 17 Jan 2025 11:03:03 +0000 Subject: [PATCH 17/35] fix: set service to ukhpi dataset if not already set --- app/models/latest_values_command.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/models/latest_values_command.rb b/app/models/latest_values_command.rb index e716f253..a5990031 100644 --- a/app/models/latest_values_command.rb +++ b/app/models/latest_values_command.rb @@ -23,7 +23,8 @@ def service_api(service) # rubocop:disable Metrics/AbcSize, Metrics/MethodLength log_fields = {} service = nil if service.blank? begin - service || dataset(:ukhpi) + # Set service to ukhpi dataset if not already set + service ||= dataset(:ukhpi) log_fields[:message] = 'Connected to UK HPI service' log_type = 'info' rescue Faraday::ConnectionFailed => e From c0cc708729061e201f9ac8a084b80b3861155079 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Fri, 17 Jan 2025 11:04:25 +0000 Subject: [PATCH 18/35] refactor: move start to before any query mods are run --- app/models/latest_values_command.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/latest_values_command.rb b/app/models/latest_values_command.rb index a5990031..ad5f1e15 100644 --- a/app/models/latest_values_command.rb +++ b/app/models/latest_values_command.rb @@ -58,6 +58,7 @@ def service_api(service) # rubocop:disable Metrics/AbcSize, Metrics/MethodLength def run_query(hpi) # rubocop:disable Metrics/AbcSize, Metrics/MethodLength log_fields = {} + start = Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) success = true query = add_date_range_constraint(base_query) @@ -65,7 +66,6 @@ def run_query(hpi) # rubocop:disable Metrics/AbcSize, Metrics/MethodLength query = add_sort_constraint(query) query = add_limit_constraint(query) - start = Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) begin @results = hpi.query(query) time_taken = (Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) - start) / 1000 From 4001f092e31da7562f1d79ee0e6d3f9e3ac30405 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Fri, 17 Jan 2025 11:05:15 +0000 Subject: [PATCH 19/35] refactor: updated logging verb based on recent discussions --- app/models/latest_values_command.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/latest_values_command.rb b/app/models/latest_values_command.rb index ad5f1e15..9a2c6d42 100644 --- a/app/models/latest_values_command.rb +++ b/app/models/latest_values_command.rb @@ -69,10 +69,10 @@ def run_query(hpi) # rubocop:disable Metrics/AbcSize, Metrics/MethodLength begin @results = hpi.query(query) time_taken = (Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) - start) / 1000 - message = format("Processing Data Services API query: '#{query.to_json}' in %.0fms", time_taken) + message = format("Completed Data Services API query: '#{query.to_json}' in %.0fms", time_taken) log_fields[:duration] = time_taken log_fields[:message] = message - log_fields[:request_status] = 'processing' + log_fields[:request_status] = 'completed' log_type = 'info' rescue NoMethodError => e log_fields[:message] = "Application failed with: NoMethodError: #{e.inspect}" From 358612f4801b7dad4173460a79634317244288e4 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Fri, 7 Feb 2025 16:41:12 +0000 Subject: [PATCH 20/35] build: Add dynamic log level configuration - Introduced log level setting based on environment variable. - Default to 'debug' in development and 'info' in production/test. - Ensures consistent logging behavior across environments. --- config/environments/development.rb | 3 +++ config/environments/production.rb | 3 +++ config/environments/test.rb | 3 +++ 3 files changed, 9 insertions(+) diff --git a/config/environments/development.rb b/config/environments/development.rb index fcaae2d8..ee827384 100644 --- a/config/environments/development.rb +++ b/config/environments/development.rb @@ -53,4 +53,7 @@ # API location can be specified in the environment but defaults to the dev service config.api_service_url = ENV.fetch('API_SERVICE_URL', 'http://localhost:8888') + + # Set the log level to the value of the LOG_LEVEL environment variable, or 'debug' if not set + config.log_level = ENV.fetch('LOG_LEVEL', 'debug').to_sym end diff --git a/config/environments/production.rb b/config/environments/production.rb index 38d2a375..faf7bc79 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -92,4 +92,7 @@ # API location is specified in the environment variable API_SERVICE_URL config.api_service_url = ENV.fetch('API_SERVICE_URL', nil) + + # Set the log level to the value of the LOG_LEVEL environment variable, or 'info' if not set + config.log_level = ENV.fetch('LOG_LEVEL', 'info').to_sym end diff --git a/config/environments/test.rb b/config/environments/test.rb index cfe3a61f..b487fd77 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -47,4 +47,7 @@ # API location can be specified in the environment # But defaults to the dev service config.api_service_url = ENV.fetch('API_SERVICE_URL', 'http://localhost:8080') + + # Set the log level to the value of the LOG_LEVEL environment variable, or 'info' if not set + config.log_level = ENV.fetch('LOG_LEVEL', 'info').to_sym end From 6c8d7c5cde895ae1973e67719b3deda7b61eddb2 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Fri, 7 Feb 2025 16:41:51 +0000 Subject: [PATCH 21/35] style: Updates Puma stats display with details toggle Added a collapsible section for Puma stats to improve UI. Now devs can expand or collapse the stats view, making it cleaner and more user-friendly. Adjusted padding for better spacing too. --- app/views/common/_puma_stats.html.haml | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/app/views/common/_puma_stats.html.haml b/app/views/common/_puma_stats.html.haml index 2efbd0be..5324ae78 100644 --- a/app/views/common/_puma_stats.html.haml +++ b/app/views/common/_puma_stats.html.haml @@ -1,5 +1,8 @@ -%div - %pre - %code - = "[#{JSON.pretty_generate(puma_stats: JSON.parse(Puma.stats), time: Time.now)}]" +%div{style: 'padding: 1rem;'} + %details + %summary + Puma Stats + %pre + %code + = "[#{JSON.pretty_generate(puma_stats: JSON.parse(Puma.stats), time: Time.now)}]" From 20b825703ac77cb1764c9d33d3b478e5930065b8 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Fri, 7 Feb 2025 16:42:36 +0000 Subject: [PATCH 22/35] fix: prevent constant redefinition warnings - Added conditional assignment to prevent warnings for MUTATIONS. - Updated UserParam struct definition to avoid redefinition warning. - Modified LocationSearchType struct initialization to suppress warnings. --- app/lib/welsh_grammar.rb | 3 ++- app/models/concerns/user_choices.rb | 3 ++- app/models/locations.rb | 4 +++- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/app/lib/welsh_grammar.rb b/app/lib/welsh_grammar.rb index 72389598..52da38c9 100644 --- a/app/lib/welsh_grammar.rb +++ b/app/lib/welsh_grammar.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true -MUTATIONS = { +# Prevent warnings about constant alrady being initialized +MUTATIONS ||= { 'yn' => [ Mutation.new(/b(.*)/i, 'm\1', 'ym'), diff --git a/app/models/concerns/user_choices.rb b/app/models/concerns/user_choices.rb index 65e69519..d1bc16c5 100644 --- a/app/models/concerns/user_choices.rb +++ b/app/models/concerns/user_choices.rb @@ -3,7 +3,8 @@ # Shared functionality for user models that provide access to the users' # choices articulated via the params in the incoming request module UserChoices - Struct.new('UserParam', :default_value, :array?, :alias) + # prevent warning about UserParam being redefined + UserParam ||= Struct.new('UserParam', :default_value, :array?, :alias) attr_reader :params diff --git a/app/models/locations.rb b/app/models/locations.rb index ba2f90e4..3fc73c3d 100644 --- a/app/models/locations.rb +++ b/app/models/locations.rb @@ -6,7 +6,9 @@ class Locations extend LocationsTable - Struct.new('LocationSearchType', :label, :rdf_types) + # Prevent a warning about LocationSearchType being redefined + LocationSearchType ||= Struct.new('LocationSearchType', :label, :rdf_types) + LOCATION_SEARCH_TYPES = { 'country' => Struct::LocationSearchType.new('Country', [ 'http://data.ordnancesurvey.co.uk/ontology/admingeo/EuropeanRegion' From 317402a644b763f15e5176c34f7bfd25742f5d0c Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Fri, 7 Feb 2025 16:43:16 +0000 Subject: [PATCH 23/35] refactor: Add Sentry logging enhancements - Moved Sentry initialization to run on DOMContentLoaded. - Added debug and environment settings for better logging. - Expanded error reporting environments to include preprod and dev. - Ignored specific non-critical exceptions in error tracking. - Set release version dynamically from a version constant. - Adjusted sample rates for traces and profiles based on environment. --- app/javascript/packs/ukhpi_vue.js | 18 ++++++++++++------ config/initializers/sentry.rb | 28 ++++++++++++++++++++++++---- 2 files changed, 36 insertions(+), 10 deletions(-) diff --git a/app/javascript/packs/ukhpi_vue.js b/app/javascript/packs/ukhpi_vue.js index 2b6297fe..d71487bd 100644 --- a/app/javascript/packs/ukhpi_vue.js +++ b/app/javascript/packs/ukhpi_vue.js @@ -49,13 +49,19 @@ if (i18n.locale === 'cy') { } document.addEventListener('DOMContentLoaded', () => { + // Sentry.io logging + Sentry.init({ + dsn: 'https://1150348b449a444bb3ac47ddd82b37c4@sentry.io/251669', + debug: process.env.NODE_ENV === 'development', + environment: process.env.NODE_ENV, + integrations: [ + new Integrations.Vue({ Vue, attachProps: true }) + ], + release: window.ukhpi.version || '1.0.0', + ignoreErrors: ['Non-Error promise rejection captured'] + }) + if (process.env.NODE_ENV === 'production') { - // Sentry.io logging - Sentry.init({ - dsn: 'https://1150348b449a444bb3ac47ddd82b37c4@sentry.io/251669', - integrations: [new Integrations.Vue({ Vue, attachProps: true })], - release: window.ukhpi.version || 'unknown-version' - }) Sentry.configureScope(scope => { scope.setTag('app', 'ukhpi-js') }) diff --git a/config/initializers/sentry.rb b/config/initializers/sentry.rb index 3147c0e2..a3826e74 100644 --- a/config/initializers/sentry.rb +++ b/config/initializers/sentry.rb @@ -1,14 +1,34 @@ # frozen_string_literal: true +require 'version' + Rails.application.reloader.to_prepare do if ENV['SENTRY_API_KEY'] Sentry.init do |config| + # https://docs.sentry.io/platforms/ruby/configuration/options/#breadcrumbs-logger + config.breadcrumbs_logger = %i[sentry_logger monotonic_active_support_logger http_logger] + # The DSN tells the SDK where to send events. config.dsn = ENV['SENTRY_API_KEY'] - config.environment = ENV.fetch('DEPLOYMENT_ENVIRONMENT') { Rails.env } - config.enabled_environments = %w[production] + # Only report errors in these environments: + config.enabled_environments = %w[production prod preprod dev] + # Ignore exceptions that are not useful to us + config.excluded_exceptions += [ + 'ActionController::BadRequest', + 'ActionController::RoutingError', + 'ActiveRecord::RecordNotFound' + ] + # Set the environment name from the DEPLOYMENT_ENVIRONMENT environment variable + config.environment = ENV.fetch('DEPLOYMENT_ENVIRONMENT', Rails.env) + ## Default to only reporting info, warnings and errors to Sentry + config.logger.level = Rails.application.config.log_level || :info + # Set the release version to the current version config.release = Version::VERSION - config.breadcrumbs_logger = %i[active_support_logger http_logger] - config.excluded_exceptions += ['ActionController::BadRequest'] + # Set traces_sample_rate to 1.0 to capture 100% of transactions for tracing. + # Sentry recommends adjusting this value in production hence the ternary operator. + config.traces_sample_rate = Rails.env.development? ? 1.0 : 0.1 + # Set profiles_sample_rate to profile 100% of sampled transactions. + # Sentry recommends adjusting this value in production hence the ternary operator. + config.profiles_sample_rate = Rails.env.production? ? 1.0 : 0.1 end end end From 9b2cdd4d8a05081fd3edd5f7cffc24d31e63f714 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Fri, 7 Feb 2025 16:44:03 +0000 Subject: [PATCH 24/35] fix: Refactor query logging and execution timing - Updated log messages for clarity and consistency. - Changed time measurement from microseconds to milliseconds in the main query method. - Enhanced logging structure with more detailed status information. - Removed unnecessary logging in helper methods to streamline code. --- app/services/query_command.rb | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-) diff --git a/app/services/query_command.rb b/app/services/query_command.rb index 07143e03..6a4da3bb 100644 --- a/app/services/query_command.rb +++ b/app/services/query_command.rb @@ -12,15 +12,17 @@ def initialize(user_selections) end # Perform the UKHPI query encapsulated by this command object - # @param service Optional API service end-point to use. Defaults to the UKHPI - # API service endpoint + # @param service Optional API service endpoint to use. + # Defaults to the UKHPI API service endpoint def perform_query(service = nil) log_fields = {} - time_taken = execute_query(service, query) - log_fields[:message] = format("Completed Data Services API roundtrip took %.0fms\n", time_taken) + time_taken = execute_query(service, query) / 1000 + msg = format('Completed Data Services API request from UKHPI service in %.0f ms', time_taken) + log_fields[:message] = msg log_fields[:request_status] = 'completed' - log_fields[:duration] = time_taken - Rails.logger.info(log_fields) + log_fields[:request_time] = time_taken + log_fields[:status] = Rack::Utils::SYMBOL_TO_STATUS_CODE[:ok] + Rails.logger.info(JSON.generate(log_fields)) end # @return True if this a query execution command @@ -36,14 +38,10 @@ def explain_query_command? private # Construct the DsAPI query that matches the given user constraints - def build_query - log_fields = {} - log_fields[:message] = 'Received Data Services API query' - log_fields[:request_status] = 'received' + def build_query # rubocop:disable Metrics/AbcSize, Metrics/MethodLength query = add_date_range_constraint(base_query) query1 = add_location_constraint(query) add_sort(query1) - Rails.logger.info(log_fields) end def api_service(service) @@ -55,14 +53,10 @@ def default_service end # Run the given query, stash the results, and return time taken in microseconds. - def execute_query(service, query) - log_fields = {} - log_fields[:message] = 'Processing Data Services API query' - log_fields[:request_status] = 'processing' + def execute_query(service, query) # rubocop:disable Metrics/AbcSize, Metrics/MethodLength start = Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) @results = api_service(service).query(query) - Rails.logger.info(log_fields) - (Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) - start) / 1000 + (Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) - start) end def add_date_range_constraint(query) From 788e030d063a56f5b35a5c3bd666c32dd6bff92b Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Thu, 13 Feb 2025 11:52:00 +0000 Subject: [PATCH 25/35] build: Add pre-commit and pre-push hooks - Introduced a pre-commit hook to run Rubocop on staged Ruby files. - Added logic to automatically stage changes made by Rubocop. - Implemented a pre-push hook to run Rails tests before pushing. - Both hooks skip checks for specific branch names. --- .githooks/pre-commit | 55 ++++++++++++++++++++++++++++++++++++++++++ .githooks/pre-push | 57 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+) create mode 100755 .githooks/pre-commit create mode 100755 .githooks/pre-push diff --git a/.githooks/pre-commit b/.githooks/pre-commit new file mode 100755 index 00000000..92df5d4c --- /dev/null +++ b/.githooks/pre-commit @@ -0,0 +1,55 @@ +#!/bin/sh +# caveat: this script assumes all modifications to a file were staged in the commit +# beware if you are in the habit of committing only partial modifications to a file: +# THIS HOOK WILL ADD ALL MODIFICATIONS TO A FILE TO THE COMMIT IF ANY FILE WAS CHANGED BY LINTING + +list="issue spike task" + +listRE="^($(printf '%s\n' "$list" | tr ' ' '|'))/" + +BRANCH_NAME=$(git branch --show-current | grep -E "$listRE" | sed 's/* //') + +printf '\n\033[0;105mChecking "%s"... \033[0m\n' "$BRANCH_NAME" + +if echo "$BRANCH_NAME" | grep -q '^(rebase)|(production)*$'; then + printf '\n\033[0;32mNo checks necessary on "%s", pushing now... ๐ŸŽ‰\033[0m\n' "$BRANCH_NAME" + exit 0 +fi + +RUBY_FILES="$(git diff --diff-filter=d --name-only --cached | grep -E '(Gemfile|Rakefile|\.(rb|rake|ru))$')" +PRE_STATUS="$(git status | wc -l)" + +WORK_DONE=0 + +if [ -z "$RUBY_FILES" ]; then + printf '\n\033[0;31mThere are currently no updated files in "%s". ๐Ÿคจ\033[0m\n' "$BRANCH_NAME" +fi + + +if [ -n "$RUBY_FILES" ]; then + printf '\n\033[0;33mRunning Rubocop...\033[0m\n' + bundle exec rubocop --autocorrect "$RUBY_FILES" + RUBOCOP_EXIT_CODE=$? + WORK_DONE=1 +else + printf '\n\033[0;32mContinuing as there are no changes to check... ๐ŸŽ‰\033[0m\n' + RUBOCOP_EXIT_CODE=0 +fi + +POST_STATUS="$(git status | wc -l)" + +if [ ! $RUBOCOP_EXIT_CODE -eq 0 ]; then + git reset HEAD + printf '\n\033[0;31mLinting has uncorrectable errors; please fix and restage your commit. ๐Ÿ˜–\033[0m\n' + exit 1 +fi + +if [ "$PRE_STATUS" != "$POST_STATUS" ]; then + git add "$RUBY_FILES" +fi + +if [ $WORK_DONE -eq 1 ]; then + printf '\n\033[0;32mLinting completed successfully! ๐ŸŽ‰\033[0m\n' +fi + +exit 0 diff --git a/.githooks/pre-push b/.githooks/pre-push new file mode 100755 index 00000000..18deee74 --- /dev/null +++ b/.githooks/pre-push @@ -0,0 +1,57 @@ +#!/bin/bash + +list="issue spike task" + +listRE="^($(printf '%s\n' "$list" | tr ' ' '|'))/" + +BRANCH_NAME=$(git branch --show-current | grep -E "$listRE" | sed 's/* //') + +printf '\n\033[0;105mChecking "%s"... \033[0m\n' "$BRANCH_NAME" + +if echo "$BRANCH_NAME" | grep -q '^(rebase)|(production)*$'; then + printf '\n\033[0;32mNo checks necessary on "%s", pushing now... ๐ŸŽ‰\033[0m\n' "$BRANCH_NAME" + exit 0 +fi + +# Check for existence of "new or modified" test files +TEST_FILES="$(git diff --diff-filter=ACDM --name-only --cached | grep -E '(./test/*)$')" +RUBY_FILES="$(git diff --diff-filter=ACDM --name-only --cached | grep -E '(_test\.rb)$')" +PRE_STATUS="$(git status | wc -l)" + +WORK_DONE=0 + +if [ -z "$TEST_FILES" ]; then + printf '\n\033[0;31mThere are currently no new tests found in "%s". ๐Ÿคจ\033[0m\n' "$BRANCH_NAME" + printf '\n\033[0;31mContinuing without new tests... ๐Ÿ˜–\033[0m\n' +fi + + +if [ -n "$RUBY_FILES" ]; then + printf '\nRunning Rails Tests...' + bundle exec rails test + RUBY_TEST_EXIT_CODE=$? + WORK_DONE=1 +else + RUBY_TEST_EXIT_CODE=0 +fi + +if [ -n "$RUBY_FILES" ]; then + printf '\nRunning System Tests...' + bundle exec rails test:system + RUBY_SYSTEM_EXIT_CODE=$? + WORK_DONE=1 +else + RUBY_SYSTEM_EXIT_CODE=0 +fi + + +if [ ! $RUBY_TEST_EXIT_CODE -eq 0 ] || [ ! $RUBY_SYSTEM_EXIT_CODE -eq 0 ]; then + printf '\n\033[0;31mCannot push, tests are failing. Use --no-verify to force push. ๐Ÿ˜–\033[0m\n' + exit 1 +fi + +if [ $WORK_DONE = 1 ]; then + printf '\n\033[0;32mAll tests are green, pushing... ๐ŸŽ‰\033[0m\n' +fi + +exit 0 From 3fd37f5321935e3c4ac5919098f1e8bee4221e39 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Thu, 13 Feb 2025 12:20:17 +0000 Subject: [PATCH 26/35] style: Update list styles in common stylesheet Removed default list styles for ordered and unordered lists, including navigation. Now they have no bullets or markers, making them cleaner and more consistent. --- app/assets/stylesheets/_lr-common.scss | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/app/assets/stylesheets/_lr-common.scss b/app/assets/stylesheets/_lr-common.scss index c814c092..30625417 100644 --- a/app/assets/stylesheets/_lr-common.scss +++ b/app/assets/stylesheets/_lr-common.scss @@ -25,6 +25,12 @@ $lr-green-nav-darkened: darken( $lr-green-nav, 20% ); /* settings */ +ol, ul, nav ol, nav ul { + list-style: unset; + list-style-type: none; + list-style-position: inside; +} + /* elements */ a { From d7c0b7aa357865edb7ec6ddb32e556e83198d3b4 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Thu, 13 Feb 2025 12:37:32 +0000 Subject: [PATCH 27/35] refactor: Improve logging for request errors - Renamed the logging method for clarity. - Updated log to skip OK responses. - Enhanced query string handling in logs. - Added request time in milliseconds to log fields. - Sorted log fields before passing them to the response logger. --- app/controllers/application_controller.rb | 32 +++++++++++------------ 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 023eb9aa..b9945fff 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -36,7 +36,7 @@ def log_request_result start = Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) yield duration = Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) - start - detailed_request_log(duration) + detailed_log_result(duration) end # Handle specific types of exceptions and render the appropriate error page @@ -85,7 +85,7 @@ def render_500(_exception = nil) # rubocop:disable Naming/VariableNumber render_error(500) end - def render_error(status, sentry_code = nil) # rubocop:disable Metrics/AbcSize + def render_error(status, sentry_code = nil) reset_response status = Rack::Utils::SYMBOL_TO_STATUS_CODE[status] if status.is_a?(Symbol) @@ -98,7 +98,7 @@ def render_error(status, sentry_code = nil) # rubocop:disable Metrics/AbcSize end end - def render_html_error_page(status, sentry_code) # rubocop:disable Metrics/MethodLength + def render_html_error_page(status, sentry_code) render 'exceptions/error_page', locals: { status: status, sentry_code: sentry_code }, layout: true, @@ -111,24 +111,24 @@ def reset_response private - # rubocop:disable Metrics/AbcSize, Metrics/MethodLength, Layout/LineLength - def detailed_request_log(duration) - env = request.env + # rubocop:disable Metrics/AbcSize, Metrics/MethodLength, Layout/LineLength, Metrics/PerceivedComplexity, Metrics/CyclomaticComplexity + def detailed_log_result(duration) + # prevent OK responses from being logged as they're logged elsewhere + return unless response&.status != 200 + env = request.env + query = env['QUERY_STRING'] || URI.parse(env['REQUEST_URI']).query log_fields = { - accept: env['HTTP_ACCEPT'], - body: request.body.gets&.gsub("\n", '\n'), - duration: duration, - forwarded_for: env['X_FORWARDED_FOR'], message: response.message || Rack::Utils::HTTP_STATUS_CODES[response.status], - path: env['REQUEST_PATH'], - query_string: env['QUERY_STRING'], + path: env['REQUEST_PATH'] || URI.parse(env['REQUEST_URI']).path, request_id: env['X_REQUEST_ID'], - request_uri: env['REQUEST_URI'], + request_time: (duration / 1000) || env['REQUEST_TIME'], # in milliseconds method: request.method, status: response.status } + log_fields[:query_string] = query if query.present? + if env['HTTP_USER_AGENT'] && Rails.env.production? log_fields[:user_agent] = env['HTTP_USER_AGENT'] end @@ -138,7 +138,7 @@ def detailed_request_log(duration) log_fields[:backtrace] = env['action_dispatch.backtrace'].join("\n") unless Rails.env.production? end - log_response(response.status, log_fields) + log_response(response.status, log_fields.sort.to_h) end # rubocop:enable Metrics/AbcSize, Metrics/MethodLength, Layout/LineLength @@ -159,7 +159,7 @@ def log_response(status, error_log) # @param [exc] exp the exception that caused the error # @return [ActiveSupport::Notifications::Event] provides an object-oriented # interface to the event - # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity, Metrics/MethodLength + # rubocop:disable Metrics/AbcSize def instrument_internal_error(exc, status = nil) err = { message: exc&.message || exc, @@ -180,5 +180,5 @@ def instrument_internal_error(exc, status = nil) # Return the event id for the internal error event sevent&.event_id end - # rubocop:enable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity, Metrics/MethodLength + # rubocop:enable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity end From 9b9cd9298cc0d12d8e5f93f4d1fff67825090ded Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Thu, 13 Feb 2025 12:44:39 +0000 Subject: [PATCH 28/35] feat: Add logging helper for API requests Introduced a new class to handle logging of API requests. Key features include: - Logs request details like method, path, and query string. - Supports different log levels (default is 'info'). - Automatically formats and sorts log fields before outputting. - Includes conditional logging for development environment. --- app/lib/logging_helper.rb | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 app/lib/logging_helper.rb diff --git a/app/lib/logging_helper.rb b/app/lib/logging_helper.rb new file mode 100644 index 00000000..460c08ea --- /dev/null +++ b/app/lib/logging_helper.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +# :nodoc: +class LoggingHelper + def self.log_request(fields, type = 'info') # rubocop:disable Metrics/AbcSize, Metrics/MethodLength, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity + puts "\n" if Rails.env.development? && Rails.logger.debug? # rubocop:disable Rails/Output + + # Extract service and params from fields hash + service = fields.delete(:service) if fields.key(:service) + params = fields.delete(:params) if fields.key(:params) + + fields[:message] ||= 'Received Data Services API request from the UKHPI service' + fields[:method] ||= 'GET' + fields[:path] ||= URI.parse(service.data_api).path if service + # fields[:request_path] ||= URI.parse(service.data_api).path if service + fields[:query_string] ||= JSON.generate(params) if params + fields[:request_status] ||= 'received' + fields[:status] ||= Rack::Utils::SYMBOL_TO_STATUS_CODE[:ok] + + Rails.logger.send(type) { JSON.generate(fields.sort.to_h) } unless fields.empty? + Rails.logger.flush if Rails.logger.respond_to?(:flush) + end +end From e75d691d87796ada9fe0ac50a2c0006cc786ad71 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Thu, 13 Feb 2025 12:47:19 +0000 Subject: [PATCH 29/35] refactor: Update Rubocop command in pre-commit hook Switched from `--autocorrect` to `-A` for Rubocop. This change simplifies the command while maintaining functionality. --- .githooks/pre-commit | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.githooks/pre-commit b/.githooks/pre-commit index 92df5d4c..c65a3952 100755 --- a/.githooks/pre-commit +++ b/.githooks/pre-commit @@ -28,7 +28,7 @@ fi if [ -n "$RUBY_FILES" ]; then printf '\n\033[0;33mRunning Rubocop...\033[0m\n' - bundle exec rubocop --autocorrect "$RUBY_FILES" + bundle exec rubocop -A "$RUBY_FILES" RUBOCOP_EXIT_CODE=$? WORK_DONE=1 else From 1b04e857562b2c317c15fed8e1d9ae802211dcd1 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Thu, 13 Feb 2025 13:16:56 +0000 Subject: [PATCH 30/35] feat: Add logging for API requests and responses - Implemented request logging in multiple controllers. - Enhanced error handling with detailed logs in the service layer. - Streamlined log messages to include execution time and status. - Updated query command to log completion details. --- app/controllers/browse_controller.rb | 1 + app/controllers/compare_controller.rb | 5 ++ app/models/latest_values_command.rb | 75 ++++++++++++++------------- app/services/query_command.rb | 39 ++++++++++---- 4 files changed, 76 insertions(+), 44 deletions(-) diff --git a/app/controllers/browse_controller.rb b/app/controllers/browse_controller.rb index f3ff76a2..6ea8c5ed 100644 --- a/app/controllers/browse_controller.rb +++ b/app/controllers/browse_controller.rb @@ -14,6 +14,7 @@ def show elsif explain_non_json?(user_selections) redirect_to_html_view(user_selections) else + LoggingHelper.log_request({ params: params }) render_view_state(setup_view_state(user_selections)) end end diff --git a/app/controllers/compare_controller.rb b/app/controllers/compare_controller.rb index a66da0ca..a169ec51 100644 --- a/app/controllers/compare_controller.rb +++ b/app/controllers/compare_controller.rb @@ -41,6 +41,11 @@ def perform_query(user_compare_selections) # rubocop:disable Metrics/MethodLengt ) user_compare_selections.selected_locations.each do |location| + msg = 'Received Data Services API request from UKHPI service' + msg += " for #{location.label}" if location + log_fields = { params: base_selection.with('location', location.uri) } + log_fields[:message] = msg + LoggingHelper.log_request(log_fields) query_results[location.label] = query_with(base_selection, location) end diff --git a/app/models/latest_values_command.rb b/app/models/latest_values_command.rb index 9a2c6d42..30835174 100644 --- a/app/models/latest_values_command.rb +++ b/app/models/latest_values_command.rb @@ -1,19 +1,13 @@ # frozen_string_literal: true # Command object to query the API for the latest available values -class LatestValuesCommand +class LatestValuesCommand # rubocop:disable Metrics/ClassLength include DataService attr_reader :results def perform_query(service = nil) - log_fields = {} - log_fields[:message] = 'Received Data Services API query' - log_fields[:request_status] = 'received' - - Rails.logger.info(JSON.generate(log_fields)) hpi = service_api(service) - (hpi && run_query(hpi)) || no_service end @@ -21,43 +15,47 @@ def perform_query(service = nil) def service_api(service) # rubocop:disable Metrics/AbcSize, Metrics/MethodLength, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity log_fields = {} + log_type = 'error' + service = nil if service.blank? begin # Set service to ukhpi dataset if not already set service ||= dataset(:ukhpi) - log_fields[:message] = 'Connected to UK HPI service' - log_type = 'info' + rescue Faraday::ConnectionFailed => e - log_fields[:message] = 'Failed to connect to UK HPI service' - log_fields[:status] = e.status + log_fields[:message] = 'Failed to connect to UKHPI service' log_fields[:body] = e.message log_fields[:backtrace] = e&.backtrace&.join("\n") if Rails.env.development? - log_fields[:request_status] = 'error' + log_fields[:request_status] = log_type + log_fields[:status] = e.status service = nil rescue DataServicesApi::ServiceException => e - log_fields[:message] = 'Failed to get response from UK HPI service' - log_fields[:status] = e.status + log_fields[:message] = 'Failed to get response from UKHPI service' log_fields[:body] = e.service_message - log_fields[:request_status] = 'error' + log_fields[:request_status] = log_type + log_fields[:status] = e.status service = nil rescue RuntimeError => e log_fields[:message] = "Runtime error #{e.inspect}" - log_fields[:status] = e.status log_fields[:body] = "Caused by: #{e.cause} in " if e.cause - log_fields[:body] += e.class + log_fields[:body] += "\r\n(#{e.class})" if Rails.application.config.log_level == :debug log_fields[:backtrace] = e&.backtrace&.join("\n") if Rails.env.development? - log_fields[:request_status] = 'error' + log_fields[:request_status] = log_type + log_fields[:status] = e.status service = nil end - Rails.logger.send(log_type) { log_fields } + + LoggingHelper.log_request(log_fields, log_type) unless log_fields.empty? + service end - def run_query(hpi) # rubocop:disable Metrics/AbcSize, Metrics/MethodLength + def run_query(hpi) # rubocop:disable Metrics/AbcSize, Metrics/MethodLength, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity log_fields = {} + log_type = 'error' start = Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) success = true @@ -66,31 +64,38 @@ def run_query(hpi) # rubocop:disable Metrics/AbcSize, Metrics/MethodLength query = add_sort_constraint(query) query = add_limit_constraint(query) + # Log the initial request received, passing in the service and query params + LoggingHelper.log_request({ service: hpi, params: query }) + begin @results = hpi.query(query) - time_taken = (Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) - start) / 1000 - message = format("Completed Data Services API query: '#{query.to_json}' in %.0fms", time_taken) - log_fields[:duration] = time_taken - log_fields[:message] = message - log_fields[:request_status] = 'completed' + + msg = 'completed Data Services API request from the UKHPI service' log_type = 'info' rescue NoMethodError => e - log_fields[:message] = "Application failed with: NoMethodError: #{e.inspect}" - log_fields[:request_status] = 'error' - log_type = 'error' + msg = "application failed with: NoMethodError: #{e.inspect}" + log_fields[:status] = 405 success = false rescue ArgumentError => e - log_fields[:message] = "Data Services API query failed with: ArgumentError: #{e.inspect}" - log_fields[:request_status] = 'error' - log_type = 'error' + msg = "Data Services API request failed with: ArgumentError: #{e.inspect}" + log_fields[:status] = 422 success = false rescue RuntimeError => e - log_fields[:message] = "Data Services API query failed with: #{e.inspect}" - log_fields[:request_status] = 'error' - log_type = 'error' + msg = "Data Services API request failed with: #{e.inspect}" + log_fields[:status] = 400 success = false end - Rails.logger.send(log_type) { JSON.generate(log_fields) } + # Calculate the time taken to execute the query and pass in the details to be logged + time_taken = (Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) - start) / 1000 + msg += format(' in %.0f ms', time_taken) if time_taken.positive? + log_fields[:message] = msg.upcase_first + log_fields[:request_status] = success ? 'completed' : 'error' + log_fields[:request_time] = time_taken + log_fields[:status] = Rack::Utils::SYMBOL_TO_STATUS_CODE[:ok] if success + + # Log the final request status and response + LoggingHelper.log_request(log_fields, log_type) unless log_fields.empty? + puts "\n" if Rails.env.development? && Rails.logger.debug? success end diff --git a/app/services/query_command.rb b/app/services/query_command.rb index 6a4da3bb..3dca2cc1 100644 --- a/app/services/query_command.rb +++ b/app/services/query_command.rb @@ -6,23 +6,26 @@ class QueryCommand attr_reader :user_selections, :results, :query + # Create a new UKHPI query command object + # @param [UserSelections] user_selections the user's selections + # @return [QueryCommand] the new command object def initialize(user_selections) @user_selections = user_selections @query = build_query end # Perform the UKHPI query encapsulated by this command object - # @param service Optional API service endpoint to use. + # @param [DataServicesApi::Service] service the API service to use # Defaults to the UKHPI API service endpoint - def perform_query(service = nil) - log_fields = {} + def perform_query(service = nil) # rubocop:disable Metrics/AbcSize + log_fields = { message: 'Completed Data Services API request from the UKHPI service' } time_taken = execute_query(service, query) / 1000 - msg = format('Completed Data Services API request from UKHPI service in %.0f ms', time_taken) - log_fields[:message] = msg + log_fields[:message] += format(' in %.0f ms', time_taken) if time_taken.positive? log_fields[:request_status] = 'completed' log_fields[:request_time] = time_taken log_fields[:status] = Rack::Utils::SYMBOL_TO_STATUS_CODE[:ok] - Rails.logger.info(JSON.generate(log_fields)) + LoggingHelper.log_request(log_fields) unless log_fields.empty? + puts "\n" if Rails.env.development? && Rails.logger.debug? end # @return True if this a query execution command @@ -38,27 +41,35 @@ def explain_query_command? private # Construct the DsAPI query that matches the given user constraints - def build_query # rubocop:disable Metrics/AbcSize, Metrics/MethodLength + def build_query query = add_date_range_constraint(base_query) query1 = add_location_constraint(query) add_sort(query1) end + # @return [DataServicesApi::Service] the API service to use def api_service(service) @api_service ||= service || default_service end + # @return [DataServicesApi::Service] the default API service to use def default_service dataset(:ukhpi) end - # Run the given query, stash the results, and return time taken in microseconds. - def execute_query(service, query) # rubocop:disable Metrics/AbcSize, Metrics/MethodLength + # Execute the given query using the given service + # @param [DataServicesApi::Service] service the API service to use + # @param [DataServicesApi::Query] query the query to execute + # @return [Integer] the time taken to execute the query in microseconds + def execute_query(service, query) start = Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) @results = api_service(service).query(query) (Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) - start) end + # Add a date range constraint to the given query + # @param [DataServicesApi::Query] query the query to add the constraint to + # @return [DataServicesApi::Query] the modified query def add_date_range_constraint(query) from = month_year_value(user_selections.from_date) to = month_year_value(user_selections.to_date) @@ -67,19 +78,29 @@ def add_date_range_constraint(query) .le('ukhpi:refMonth', to) end + # Add a location constraint to the given query + # @param [DataServicesApi::Query] query the query to add the constraint to + # @return [DataServicesApi::Query] the modified query def add_location_constraint(query) value = DataServicesApi::Value.uri(location_uri) query.eq('ukhpi:refRegion', value) end + # Add a sort constraint to the given query + # @param [DataServicesApi::Query] query the query to add the constraint to + # @return [DataServicesApi::Query] the modified query def add_sort(query) query.sort(:up, 'ukhpi:refMonth') end + # Convert provided date to a year-month value + # @param [Date] date the date to convert + # @return [DataServicesApi::Value] the year-month value def month_year_value(date) DataServicesApi::Value.year_month(date.year, date.month) end + # @return [String] the URI of the selected location def location_uri user_selections.selected_location end From e829c2edc69a7436735d59ce6d7def5c85631be0 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Thu, 13 Feb 2025 13:20:31 +0000 Subject: [PATCH 31/35] refactor: updates to user choices and location search types - Updated struct definitions to avoid redefinition warnings - Improved formatting for readability and compliance with style guidelines - Simplified location search type initialization using constants for URLs --- app/models/concerns/user_choices.rb | 4 ++-- app/models/locations.rb | 34 ++++++++++++++--------------- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/app/models/concerns/user_choices.rb b/app/models/concerns/user_choices.rb index d1bc16c5..196f6bd1 100644 --- a/app/models/concerns/user_choices.rb +++ b/app/models/concerns/user_choices.rb @@ -3,8 +3,8 @@ # Shared functionality for user models that provide access to the users' # choices articulated via the params in the incoming request module UserChoices - # prevent warning about UserParam being redefined - UserParam ||= Struct.new('UserParam', :default_value, :array?, :alias) + # Define the user parameters that can be set by the user + Struct.new('UserParam', :default_value, :array?, :alias) attr_reader :params diff --git a/app/models/locations.rb b/app/models/locations.rb index 3fc73c3d..bd62d2af 100644 --- a/app/models/locations.rb +++ b/app/models/locations.rb @@ -7,25 +7,25 @@ class Locations extend LocationsTable # Prevent a warning about LocationSearchType being redefined - LocationSearchType ||= Struct.new('LocationSearchType', :label, :rdf_types) + LocationSearchType = Struct.new('LocationSearchType', :label, :rdf_types) + + # Formatting update to reduce line length warnings from Rubocop + EUROPEAN_REGION = ['http://data.ordnancesurvey.co.uk/ontology/admingeo/EuropeanRegion'].freeze + LOCAL_AUTHORITIES = [ + 'http://data.ordnancesurvey.co.uk/ontology/admingeo/Borough', + 'http://data.ordnancesurvey.co.uk/ontology/admingeo/District', + 'http://data.ordnancesurvey.co.uk/ontology/admingeo/LondonBorough', + 'http://data.ordnancesurvey.co.uk/ontology/admingeo/MetropolitanDistrict', + 'http://data.ordnancesurvey.co.uk/ontology/admingeo/UnitaryAuthority' + ].freeze + ENGLAND_REGION = ['http://landregistry.data.gov.uk/def/ukhpi/Region'].freeze + ENGLAND_COUNTY = ['http://data.ordnancesurvey.co.uk/ontology/admingeo/County'].freeze LOCATION_SEARCH_TYPES = { - 'country' => Struct::LocationSearchType.new('Country', [ - 'http://data.ordnancesurvey.co.uk/ontology/admingeo/EuropeanRegion' - ]), - 'local_authority' => Struct::LocationSearchType.new('Local authority', [ - 'http://data.ordnancesurvey.co.uk/ontology/admingeo/Borough', - 'http://data.ordnancesurvey.co.uk/ontology/admingeo/District', - 'http://data.ordnancesurvey.co.uk/ontology/admingeo/LondonBorough', - 'http://data.ordnancesurvey.co.uk/ontology/admingeo/MetropolitanDistrict', - 'http://data.ordnancesurvey.co.uk/ontology/admingeo/UnitaryAuthority' - ]), - 'england_region' => Struct::LocationSearchType.new('Region (England only)', [ - 'http://landregistry.data.gov.uk/def/ukhpi/Region' - ]), - 'england_county' => Struct::LocationSearchType.new('County (England only)', [ - 'http://data.ordnancesurvey.co.uk/ontology/admingeo/County' - ]) + 'country' => LocationSearchType.new('Country', EUROPEAN_REGION), + 'local_authority' => LocationSearchType.new('Local authority', LOCAL_AUTHORITIES), + 'england_region' => LocationSearchType.new('Region (England only)', ENGLAND_REGION), + 'england_county' => LocationSearchType.new('County (England only)', ENGLAND_COUNTY) }.freeze # @return The Location with the given URI, if it exists From ed579f084096b66e7ed6f0893530100a4f87f7c3 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Thu, 13 Feb 2025 13:21:14 +0000 Subject: [PATCH 32/35] chore: Update visibility and search input types - Added conditional class for hiding elements in non-dev environments. - Changed text fields to search fields for better UX. - Cleaned up HTML structure for improved readability. --- app/views/browse/_data_view.html.haml | 4 ++-- app/views/browse/_user_selections_controls.html.haml | 2 +- app/views/compare/show.html.haml | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/views/browse/_data_view.html.haml b/app/views/browse/_data_view.html.haml index 3450a198..b4c979e0 100644 --- a/app/views/browse/_data_view.html.haml +++ b/app/views/browse/_data_view.html.haml @@ -12,7 +12,7 @@ = visibility_action - if visible - .o-data-view__options.u-js-hidden + .o-data-view__options{:class => ("u-js-hidden" unless Rails.env.development?)} = t('preposition.for').titlecase = succeed ',' do = data_view.selected_location_label @@ -33,7 +33,7 @@ %a{ href: data_view.add_remove_statistic(!checked, stat) } = stat.label - .o-data-view__data-table.u-js-hidden + .o-data-view__data-table{:class => ("u-js-hidden" unless Rails.env.development?)} - table_data = data_view.as_table_data %table %thead diff --git a/app/views/browse/_user_selections_controls.html.haml b/app/views/browse/_user_selections_controls.html.haml index 27e054ba..851457bc 100644 --- a/app/views/browse/_user_selections_controls.html.haml +++ b/app/views/browse/_user_selections_controls.html.haml @@ -4,7 +4,7 @@ %p = label_tag(:'location-term', t('browse.edit.form.search_prompt')) - = text_field_tag(:'location-term') + = search_field_tag(:'location-term') %button{ type: 'submit', value: 'search', name: 'form-action' } = t('action.search') diff --git a/app/views/compare/show.html.haml b/app/views/compare/show.html.haml index f4bcaeba..fefd9320 100644 --- a/app/views/compare/show.html.haml +++ b/app/views/compare/show.html.haml @@ -8,7 +8,7 @@ %p.u-muted = t('compare.show.prompt') - .c-location-compare--non-js.u-js-hidden + .c-location-compare--non-js{:class => ("u-js-hidden" unless Rails.env.development?)} %form.c-compare__form{ action: compare_path, method: 'get' } %p %label{ for: 'select-in' } @@ -49,7 +49,7 @@ %li.c-compare__add-location %label{ for: 'location-term', id: 'location-term-label'} = t('compare.show.add_location') - %input{ id: 'location-term', type: 'text', name: 'location-term', "aria-labelledby": "location-term-label", value: @view_state.search_term } + %input{ id: 'location-term', type: 'search', name: 'location-term', "aria-labelledby": "location-term-label", value: @view_state.search_term } %button{ type: 'submit', value: 'search', name: 'form-action' } = t('action.search') @@ -70,7 +70,7 @@ %p = msg - else - %table.c-compare__results.u-js-hidden + %table.c-compare__results{:class => ("u-js-hidden" unless Rails.env.development?)} %thead %tr %th{ scope: 'col' } From f609b8ce2143e4d0c5d7ef0855d214591331bcc5 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Thu, 13 Feb 2025 13:23:06 +0000 Subject: [PATCH 33/35] build: Update pre-commit hook for Rubocop checks Refactored the pre-commit hook to run Rubocop on updated files individually instead of all at once. This change improves error handling and output clarity for each file checked. --- .githooks/pre-commit | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.githooks/pre-commit b/.githooks/pre-commit index c65a3952..2763da5d 100755 --- a/.githooks/pre-commit +++ b/.githooks/pre-commit @@ -25,10 +25,11 @@ if [ -z "$RUBY_FILES" ]; then printf '\n\033[0;31mThere are currently no updated files in "%s". ๐Ÿคจ\033[0m\n' "$BRANCH_NAME" fi - if [ -n "$RUBY_FILES" ]; then printf '\n\033[0;33mRunning Rubocop...\033[0m\n' - bundle exec rubocop -A "$RUBY_FILES" + for file in $RUBY_FILES; do + git show :"$file" | bundle exec rubocop -A --stdin "$file" + done RUBOCOP_EXIT_CODE=$? WORK_DONE=1 else From be1c41fd68ffc76d094f74c57e2366c863882fa8 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Mon, 17 Feb 2025 12:08:43 +0000 Subject: [PATCH 34/35] fix: Update version string to be immutable Made the version string immutable by freezing it. This helps prevent accidental modifications and improves performance. --- 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 4d205e68..e0a4c35c 100644 --- a/app/lib/version.rb +++ b/app/lib/version.rb @@ -5,5 +5,5 @@ module Version MINOR = 0 PATCH = 1 SUFFIX = nil - VERSION = "#{MAJOR}.#{MINOR}.#{PATCH}#{SUFFIX && ".#{SUFFIX}"}" + VERSION = "#{MAJOR}.#{MINOR}.#{PATCH}#{SUFFIX && ".#{SUFFIX}"}".freeze end From 3c6a4f39a54bdff8b65d6d8970086fb2ca5d9299 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Mon, 17 Feb 2025 12:25:49 +0000 Subject: [PATCH 35/35] docs: Update changelog for version 2.0.1 - Changed release date to February 2025 - Improved logging with call-flow diagrams for message sequences - Enhanced visibility and search input types, including conditional classes and better UX - Added pre-commit and pre-push hooks for code quality checks - Switched time measurement from microseconds to milliseconds in queries - Removed duplicate gem entry - Introduced dynamic `LOG_LEVEL` environment variable with defaults - Enhanced Sentry logging across Rails and VUE/JS implementations - Refactored UI error handling for consistent messaging --- CHANGELOG.md | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fd267c1a..caf640b0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,8 +2,24 @@ ## Unreleased -## 2.0.1 - 2025-01 - +## 2.0.1 - 2025-02 + +- (Jon) Updated the logging to utilise a call-flow diagram to display the + sequence of messages that are sent between the app and the api. + [GH-478](https://github.com/epimorphics/ukhpi/issues/478) +- (Jon) Updated visibility and search input types + - Added conditional class for hiding elements in non-dev environments. + - Changed text fields to search fields for better UX. + - Cleaned up HTML structure for improved readability. +- (Jon) Added pre-commit and pre-push hooks + - Introduced a pre-commit hook to run Rubocop on staged Ruby files. + - Implemented a pre-push hook to run Rails tests before pushing. +- (Jon) Changed time measurement from microseconds to milliseconds in the main + query method +- (Jon) Removed duplicated gem entry +- (Jon) Added dynamic `LOG_LEVEL` env variable + - Defaults to `debug` in development and `info` in production/test. +- (Jon) Enhanced Sentry logging on both Rails and VUE/JS implementations - (Jon) Refactored UI code to use if/elsif/else in error page responses to ensure a message is displayed at all times no matter what status is passed in as per best practices