From c9c9315ef96bf57b902c37cdabe65e6a804aebb7 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Tue, 27 Aug 2024 14:49:30 +0100 Subject: [PATCH 01/11] build: updated & reorganised .gitignore --- .gitignore | 50 ++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 40 insertions(+), 10 deletions(-) diff --git a/.gitignore b/.gitignore index d2dcbeb..9996cd5 100644 --- a/.gitignore +++ b/.gitignore @@ -4,22 +4,52 @@ # or operating system, you probably want to add a global ignore instead: # git config --global core.excludesfile '~/.gitignore_global' -!/log/.keep -.byebug_history -.github-token -.npmrc -.tags +# Ignore vscode config +.vscode + +## Ignore bundler configuration: /.bundle +/vendor/bundle +/lib/bundler/man/ + +# Ignore all logfiles and tempfiles. /log/* +/tmp/* +!/log/.keep +!/tmp/.keep +C:\\nppdf32Log\\debuglog.txt + +# Ignore Byebug command history file. +.byebug_history + +# Ignore node_modules +node_modules/ + +# Ignore precompiled javascript packs /public/packs /public/packs-test -/tmp -C:\\nppdf32Log\\debuglog.txt -coverage +/public/assets + +# Ignore uploaded files in development +/storage/* +!/storage/.keep +/public/uploads + +### Rails specific ### +.byebug_history +/public/system +/coverage/ +tmp + +# Ignore files specific to the development environment fc.json fc_simple.json index-names.txt index.json -public/assets tags -test/query_test.rb + +# Ignore dot files used by environment or IDE tools +.tags +.tool-versions +.github-token +.npmrc From 29ede01f9a8f17fadc01afbf105eb1789b8e520b Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Wed, 28 Aug 2024 16:22:30 +0100 Subject: [PATCH 02/11] build: rubocop linting rule updates Added `CountAsOne: ['array', 'hash', 'heredoc', 'method_call']` to `Metrics/ClassLength` and `Metrics/MethodLength`; also includes files changed by rules, primarily removing erroneous inline disabling commands --- .rubocop.yml | 16 +++++++++------- app/helpers/download_request_helper.rb | 4 ++-- app/helpers/report_design_helper.rb | 8 +++----- app/models/report_specification.rb | 2 +- app/models/step_select_aggregation_type.rb | 2 +- app/models/step_select_county.rb | 2 +- app/models/step_select_dates.rb | 2 +- app/models/step_select_district.rb | 2 +- app/models/workflow.rb | 2 +- app/services/report_manager.rb | 4 ++-- app/services/report_manager_api.rb | 14 +++++++------- app/subscribers/api_prometheus_subscriber.rb | 2 +- config/environments/production.rb | 2 +- config/initializers/sentry.rb | 2 +- 14 files changed, 32 insertions(+), 32 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index 02529d2..9bcaf2b 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -16,9 +16,8 @@ Layout/LineLength: - config/**/* - test/**/* -Metrics/ClassLength: - Exclude: - - test/**/* +Lint/DuplicateBranch: + Enabled: false Metrics/BlockLength: Max: 30 @@ -26,18 +25,21 @@ Metrics/BlockLength: - test/**/* - lib/tasks/**/* +Metrics/ClassLength: + CountAsOne: ['array', 'hash', 'heredoc', 'method_call'] + Exclude: + - test/**/* + Metrics/MethodLength: + CountAsOne: ['array', 'hash', 'heredoc', 'method_call'] Exclude: - lib/tasks/location.rake -Style/FormatStringToken: - Enabled: false - Naming/MethodParameterName: Exclude: - test/**/* -Lint/DuplicateBranch: +Style/FormatStringToken: Enabled: false Style/OptionalBooleanParameter: diff --git a/app/helpers/download_request_helper.rb b/app/helpers/download_request_helper.rb index f020f9e..bd33ba8 100644 --- a/app/helpers/download_request_helper.rb +++ b/app/helpers/download_request_helper.rb @@ -12,7 +12,7 @@ def render_report_request(request) end end - def request_status(request) # rubocop:disable Metrics/MethodLength + def request_status(request) capture do if request.unknown? concat render_unknown_request(request) @@ -41,7 +41,7 @@ def render_failed_request(request) end end - def render_completed_request(request) # rubocop:disable Metrics/MethodLength + def render_completed_request(request) content_tag(:span, class: 'o-request--status__success') do concat('Ready: ') concat(tag(:br)) diff --git a/app/helpers/report_design_helper.rb b/app/helpers/report_design_helper.rb index c9a9130..dca7805 100644 --- a/app/helpers/report_design_helper.rb +++ b/app/helpers/report_design_helper.rb @@ -2,7 +2,7 @@ # :nodoc: module ReportDesignHelper # rubocop:disable Metrics/ModuleLength - def workflow_step_form(workflow) # rubocop:disable Metrics/MethodLength, Metrics/AbcSize + def workflow_step_form(workflow) # rubocop:disable Metrics/AbcSize step = workflow.current_step form_tag(workflow.form_action, method: 'get') do @@ -51,7 +51,7 @@ def layout_values_as_toggle_buttons(step, values, radio, cls = '') end end - def toggle_button_option(step, value, radio, single_value) # rubocop:disable Metrics/MethodLength + def toggle_button_option(step, value, radio, single_value) active = value.active? || single_value content_tag(:div, class: 'o-form-control') do @@ -115,7 +115,6 @@ def selected_area_summary(workflow) .summarise_current_value(workflow) end - # rubocop:disable Metrics/MethodLength def layout_custom_dates(delta, step, workflow) year = Time.now.year - delta content_tag(:div, class: 'row') do @@ -129,7 +128,6 @@ def layout_custom_dates(delta, step, workflow) end) end end - # rubocop:enable Metrics/MethodLength def layout_all_year(step, year, workflow) all_year = year == Time.now.year ? 'to date' : 'all year' @@ -149,7 +147,7 @@ def layout_months(step, year, workflow) layout_quarters_or_months(step.months_for(year, workflow), 'months', "#{step.param_name}[]") end - def layout_quarters_or_months(mqs, _prompt, param_name) # rubocop:disable Metrics/MethodLength + def layout_quarters_or_months(mqs, _prompt, param_name) capture do concat prompted_row( lambda { diff --git a/app/models/report_specification.rb b/app/models/report_specification.rb index f76d805..23c4b28 100644 --- a/app/models/report_specification.rb +++ b/app/models/report_specification.rb @@ -28,7 +28,7 @@ def normalize_period(report_manager) end end - def latest_quarter(report_manager) # rubocop:disable Metrics/MethodLength + def latest_quarter(report_manager) case report_manager.latest_month when 1..2 "#{report_manager.latest_year - 1}-Q4" diff --git a/app/models/step_select_aggregation_type.rb b/app/models/step_select_aggregation_type.rb index d7a5741..3bb90f7 100644 --- a/app/models/step_select_aggregation_type.rb +++ b/app/models/step_select_aggregation_type.rb @@ -14,7 +14,7 @@ def initialize super(:select_aggregation_type, :aggregate, :radio) end - def values_options(workflow) # rubocop:disable Metrics/MethodLength, Metrics/CyclomaticComplexity + def values_options(workflow) # rubocop:disable Metrics/CyclomaticComplexity case workflow.state(:areaType) when 'country' [AGGREGATE_BY_REGION, AGGREGATE_BY_COUNTY, AGGREGATE_BY_DISTRICT, diff --git a/app/models/step_select_county.rb b/app/models/step_select_county.rb index 33f0361..bb866a9 100644 --- a/app/models/step_select_county.rb +++ b/app/models/step_select_county.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true # Workflow step of selecting a county -class StepSelectCounty < StepSelectCountyOrDistrict # rubocop:disable Metrics/ClassLength +class StepSelectCounty < StepSelectCountyOrDistrict def initialize super(:select_county, :area) end diff --git a/app/models/step_select_dates.rb b/app/models/step_select_dates.rb index 14215e5..e2ee60c 100644 --- a/app/models/step_select_dates.rb +++ b/app/models/step_select_dates.rb @@ -95,7 +95,7 @@ def latest_values(workflow) ] end - def summarise_value # rubocop:disable Metrics/MethodLength + def summarise_value proc { |state_value| s = case state_value.to_sym when :ytd diff --git a/app/models/step_select_district.rb b/app/models/step_select_district.rb index 1f62a39..9625e15 100644 --- a/app/models/step_select_district.rb +++ b/app/models/step_select_district.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true # Workflow step of selecting a county -class StepSelectDistrict < StepSelectCountyOrDistrict # rubocop:disable Metrics/ClassLength +class StepSelectDistrict < StepSelectCountyOrDistrict def initialize super(:select_district, :area) end diff --git a/app/models/workflow.rb b/app/models/workflow.rb index b0e3597..f64445d 100644 --- a/app/models/workflow.rb +++ b/app/models/workflow.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true # Model encapsulating the report-generation workflow -class Workflow # rubocop:disable Metrics/ClassLength +class Workflow extend Forwardable attr_reader :step_history diff --git a/app/services/report_manager.rb b/app/services/report_manager.rb index 1de8304..4e6f2f9 100644 --- a/app/services/report_manager.rb +++ b/app/services/report_manager.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true # Service object for interacting with remote service-manager API -class ReportManager # rubocop:disable Metrics/ClassLength +class ReportManager def initialize(config = nil) return unless config @@ -74,7 +74,7 @@ def start_requests(params) # a=1&b[]=2&b[]=3 # becomes # [{a: 1, b: 2}, {a: 1, b: 3}] - def create_params_sets(params) # rubocop:disable Metrics/MethodLength + def create_params_sets(params) product = [{}] params.each do |k, v| diff --git a/app/services/report_manager_api.rb b/app/services/report_manager_api.rb index 15180e5..c730a56 100644 --- a/app/services/report_manager_api.rb +++ b/app/services/report_manager_api.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true # Encapsulates the HTTP API for the report manager -class ReportManagerApi # rubocop:disable Metrics/ClassLength +class ReportManagerApi attr_reader :instrumenter def initialize(instrumenter = ActiveSupport::Notifications) @@ -39,7 +39,7 @@ def get(http_url, options = {}) private - def get_from_api(http_url, options) # rubocop:disable Metrics/MethodLength + def get_from_api(http_url, options) conn = set_connection_timeout(create_http_connection(http_url)) conn.get do |req| @@ -65,7 +65,7 @@ def load_status_report(response) # Parse the given JSON string into a data structure. Throws an exception if # parsing fails - def parse_json(json) # rubocop:disable Metrics/MethodLength + def parse_json(json) result = nil json_hash = parser.parse(StringIO.new(json)) do |json_chunk| @@ -127,7 +127,7 @@ def report_json_failure(_json) throw msg end - def record_api_error_response(http_url, method, response, start_time) # rubocop:disable Metrics/MethodLength + def record_api_error_response(http_url, method, response, start_time) end_time = Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) ellapsed_time = end_time - start_time error_message = "API #{method} to '#{http_url}' failed: '#{response.body}'" @@ -143,7 +143,7 @@ def record_api_error_response(http_url, method, response, start_time) # rubocop: throw error_message end - def record_api_ok_response(http_url, method, response, start_time) # rubocop:disable Metrics/MethodLength + def record_api_ok_response(http_url, method, response, start_time) end_time = Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) ellapsed_time = end_time - start_time success_message = "API #{method} to '#{http_url}' succeeded: '#{response.body}'" @@ -162,7 +162,7 @@ def record_failed_connection(http_url, exception) throw "Failed to connect to '#{http_url}'" end - # rubocop:disable Metrics/MethodLength, Metrics/AbcSize + # rubocop:disable Metrics/AbcSize def log_api_response(response, start_time, url: nil, status: nil, message: '') end_time = Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) ellapsed_time = end_time - start_time @@ -186,5 +186,5 @@ def log_api_response(response, start_time, url: nil, status: nil, message: '') Rails.logger.info(JSON.generate(log_fields)) end end - # rubocop:enable Metrics/MethodLength, Metrics/AbcSize + # rubocop:enable Metrics/AbcSize end diff --git a/app/subscribers/api_prometheus_subscriber.rb b/app/subscribers/api_prometheus_subscriber.rb index b40d80f..0cc7ed7 100644 --- a/app/subscribers/api_prometheus_subscriber.rb +++ b/app/subscribers/api_prometheus_subscriber.rb @@ -4,7 +4,7 @@ class ApiPrometheusSubscriber < ActiveSupport::Subscriber attach_to :api - def response(event) # rubocop:disable Metrics/MethodLength + def response(event) response = event.payload[:response] duration = event.payload[:duration] diff --git a/config/environments/production.rb b/config/environments/production.rb index affb1d1..4c9d7ae 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -89,7 +89,7 @@ # API_SERVICE_URL should also be specified in the entrypoint.sh file and # set in the Makefile as an env variable for the docker container when run as an image. # API_SERVICE_URL is required by both Docker image and Rails - config.api_service_url = ENV['API_SERVICE_URL'] + config.api_service_url = ENV.fetch('API_SERVICE_URL', nil) # Use default paths for documentation. config.accessibility_document_path = '/accessibility' diff --git a/config/initializers/sentry.rb b/config/initializers/sentry.rb index 5d7dc91..1e6ead8 100644 --- a/config/initializers/sentry.rb +++ b/config/initializers/sentry.rb @@ -3,7 +3,7 @@ if ENV['SENTRY_API_KEY'] Sentry.init do |config| config.dsn = ENV['SENTRY_API_KEY'] - config.environment = ENV['DEPLOYMENT_ENVIRONMENT'] || Rails.env + config.environment = ENV.fetch('DEPLOYMENT_ENVIRONMENT') { Rails.env } config.enabled_environments = %w[production test] config.release = Version::VERSION config.breadcrumbs_logger = %i[active_support_logger http_logger] From 7b8ebeb8a1f7876a6caffc09c44f79e551b2b900 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Tue, 3 Sep 2024 12:15:47 +0100 Subject: [PATCH 03/11] build: updated `lr-common-styles` gem --- Gemfile | 5 +++-- Gemfile.lock | 10 +++++++--- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/Gemfile b/Gemfile index 336dde4..e8dbb5b 100644 --- a/Gemfile +++ b/Gemfile @@ -37,6 +37,7 @@ gem 'js-routes', '< 2.0' gem 'leaflet-rails' gem 'prometheus-client', '~> 4.0' gem 'puma' +gem 'puma-metrics' gem 'responders', '~> 2.0' gem 'sentry-ruby', '~> 5.2' gem 'yajl-ruby', require: 'yajl' @@ -74,11 +75,11 @@ end # TODO: While running the rails app locally for testing you can set gems to your local path # ! These "local" paths do not work with a docker image - use the repo instead # gem 'json_rails_logger', '~> 1.0.0', path: '~/Epimorphics/shared/json-rails-logger/' -# gem 'lr_common_styles', '~> 1.9.3', path: '~/Epimorphics/clients/land-registry/projects/lr_common_styles/' +# gem 'lr_common_styles', '~> 1.9', '>= 1.9.6', path: '~/Epimorphics/clients/land-registry/projects/lr_common_styles/' # rubocop:enable Layout/LineLength # TODO: In production you want to set this to the gem from the epimorphics package repo source 'https://rubygems.pkg.github.com/epimorphics' do gem 'json_rails_logger', '~> 1.0.0' - gem 'lr_common_styles', '~> 1.9', '>= 1.9.5' + gem 'lr_common_styles', '~> 1.9', '>= 1.9.6' end diff --git a/Gemfile.lock b/Gemfile.lock index 2bc0654..74ae6f1 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -232,8 +232,11 @@ GEM psych (4.0.3) stringio public_suffix (5.0.0) - puma (5.6.4) + puma (5.6.7) nio4r (~> 2.0) + puma-metrics (1.2.5) + prometheus-client (>= 0.10) + puma (>= 5.0) racc (1.7.1) rack (2.2.7) rack-test (2.1.0) @@ -364,7 +367,7 @@ GEM json lograge railties - lr_common_styles (1.9.5) + lr_common_styles (1.9.6) bootstrap-sass (~> 3.4.0) font-awesome-rails (~> 4.7.0.1) govuk_elements_rails (~> 2.0.0) @@ -402,7 +405,7 @@ DEPENDENCIES json_rails_logger (~> 1.0.0)! leaflet-rails libv8-node (>= 16.10.0.0) - lr_common_styles (~> 1.9, >= 1.9.5)! + lr_common_styles (~> 1.9, >= 1.9.6)! minitest-rails-capybara minitest-reporters minitest-spec-rails @@ -412,6 +415,7 @@ DEPENDENCIES modulejs-rails prometheus-client (~> 4.0) puma + puma-metrics rails (< 6.0.0) responders (~> 2.0) rubocop From 3ed1319b1263b083e28e7cf4d2a96e442e59c317 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Tue, 3 Sep 2024 12:17:49 +0100 Subject: [PATCH 04/11] feat: added `process_threads` metrics updated to include the metrics for filtering and sorting in the metrics dashboard as needed --- ...action_controller_prometheus_subscriber.rb | 48 ++++++++++++++++++- config/initializers/prometheus.rb | 17 +++++-- 2 files changed, 59 insertions(+), 6 deletions(-) diff --git a/app/subscribers/action_controller_prometheus_subscriber.rb b/app/subscribers/action_controller_prometheus_subscriber.rb index 63f5879..2f557b9 100644 --- a/app/subscribers/action_controller_prometheus_subscriber.rb +++ b/app/subscribers/action_controller_prometheus_subscriber.rb @@ -7,11 +7,57 @@ class ActionControllerPrometheusSubscriber < ActiveSupport::Subscriber attach_to :action_controller + # rubocop:disable Metrics/AbcSize, Metrics/MethodLength def process_action(_event) mem = GetProcessMem.new - Prometheus::Client.registry .get(:memory_used_mb) .set(mem.mb) + # description: 'Thread is aborting' + Prometheus::Client.registry + .get(:process_threads) + .set( + Thread.list.select { |thread| thread.status == 'aborting' }.count, + labels: { + status: 'aborting' + } + ) + # description: 'Thread is sleeping or waiting on I/O' + Prometheus::Client.registry + .get(:process_threads) + .set( + Thread.list.select { |thread| thread.status == 'sleep' }.count, + labels: { + status: 'sleep' + } + ) + # description: 'Thread is executing' + Prometheus::Client.registry + .get(:process_threads) + .set( + Thread.list.select { |thread| thread.status == 'run' }.count, + labels: { + status: 'run' + } + ) + # description: 'Thread is terminated normally' + Prometheus::Client.registry + .get(:process_threads) + .set( + Thread.list.select { |thread| thread.status == false }.count, + labels: { + status: 'false' + } + ) + # description: 'Thread is terminated with an exception' + Prometheus::Client.registry + .get(:process_threads) + .set( + Thread.list.select { |thread| thread.status.nil? }.count, + labels: { + status: 'nil' + } + ) end + # rubocop:enable Metrics/AbcSize, Metrics/MethodLength end diff --git a/config/initializers/prometheus.rb b/config/initializers/prometheus.rb index c802544..5691075 100644 --- a/config/initializers/prometheus.rb +++ b/config/initializers/prometheus.rb @@ -5,27 +5,27 @@ # Prometheus counters prometheus.counter( :api_status, - docstring: 'Response from back-end API', + docstring: 'Response from back-end API, labelled by status', labels: [:status] ) prometheus.counter( :api_requests, - docstring: 'Overall count of back-end API requests', + docstring: 'Overall count of back-end API requests, labelled by result', labels: [:result] ) prometheus.counter( :api_connection_failure, - docstring: 'Reasons for back-end API connection failure', + docstring: 'Reasons for back-end API connection failure, labelled by message', labels: [:message] ) prometheus.counter( :api_service_exception, - docstring: 'The response from the back-end data API was not processed', + docstring: 'The response from the back-end data API was not processed, labelled by message', labels: [:message] ) prometheus.counter( :internal_application_error, - docstring: 'Unexpected events and internal error count', + docstring: 'Unexpected events and internal error count, labelled by message', labels: [:message] ) @@ -35,6 +35,13 @@ docstring: 'Process memory usage in mb' ) +prometheus.gauge( + :process_threads, + docstring: 'The number of process threads, labelled by status', + labels: [:status], + preset_labels: { status: 'total' } +) + # Prometheus histograms prometheus.histogram( :api_response_times, From ff62b021cfc84d7613327371914216097e822bf9 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Tue, 3 Sep 2024 12:21:08 +0100 Subject: [PATCH 05/11] build: unified makefile targets mirrored makefiles from the other HMLR applications to ensure expected targets exist --- Makefile | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/Makefile b/Makefile index e8e9196..0c6aed1 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: assets clean image lint publish realclean run tag test vars +.PHONY: assets auth check clean image lint local publish realclean run tag test vars ACCOUNT?=$(shell aws sts get-caller-identity | jq -r .Account) ALPINE_VERSION?=3.13 @@ -28,23 +28,27 @@ REPO?=${ECR}/${IMAGE} GITHUB_TOKEN=.github-token BUNDLE_CFG=.bundle/config -all: image - ${BUNDLE_CFG}: ${GITHUB_TOKEN} @./bin/bundle config set --local rubygems.pkg.github.com ${GPR_OWNER}:`cat ${GITHUB_TOKEN}` ${GITHUB_TOKEN}: @echo ${PAT} > ${GITHUB_TOKEN} -assets: - @./bin/bundle config set --local without 'development' +all: image + +assets: auth + @./bin/bundle config set --local without 'development test' @./bin/bundle install @./bin/rails assets:clean assets:precompile auth: ${GITHUB_TOKEN} ${BUNDLE_CFG} +check: lint test + @echo "All checks passed." + clean: @[ -d public/assets ] && ./bin/rails assets:clobber || : + @@ rm -rf bundle coverage log node_modules image: auth @echo Building ${REPO}:${TAG} ... @@ -64,6 +68,12 @@ image: auth lint: assets @./bin/bundle exec rubocop +local: + @echo "Installing all packages ..." + @./bin/bundle install + @echo "Starting local server ..." + @./bin/rails server -p ${PORT} + publish: image @echo Publishing image: ${REPO}:${TAG} ... @docker push ${REPO}:${TAG} 2>&1 @@ -88,6 +98,7 @@ tag: @echo ${TAG} test: assets + @echo "Running tests ..." @./bin/rails test vars: From 6c7c7af017cb6770d9d466cf4577abb5054ae2b9 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Tue, 3 Sep 2024 13:23:33 +0100 Subject: [PATCH 06/11] feat: addition of `instrument_internal_error` metric Notify subscriber(s) of an internal error event with the payload of the exception once done --- app/controllers/application_controller.rb | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index c8c7613..12ac39e 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -9,6 +9,15 @@ class ApplicationController < ActionController::Base before_action :change_default_caching_policy around_action :log_request_result + + # * Set cache control headers for HMLR apps to be public and cacheable + # * Standard Reports uses a time limit of 5 minutes (300 seconds) + # Sets the default `Cache-Control` header for all requests, + # unless overridden in the action + def change_default_caching_policy + expires_in 5.minutes, public: true, must_revalidate: true if Rails.env.production? + end + def log_request_result start = Process.clock_gettime(Process::CLOCK_MONOTONIC, :microsecond) yield @@ -46,11 +55,12 @@ def detailed_request_log(duration) end # rubocop:enable Metrics/MethodLength, Metrics/AbcSize - # * Set cache control headers for HMLR apps to be public and cacheable - # * Standard Reports uses a time limit of 5 minutes (300 seconds) - # Sets the default `Cache-Control` header for all requests, - # unless overridden in the action - def change_default_caching_policy - expires_in 5.minutes, public: true, must_revalidate: true if Rails.env.production? + # Notify subscriber(s) of an internal error event with the payload of the + # exception once done + # @param [Exception] exp the exception that caused the error + # @return [ActiveSupport::Notifications::Event] provides an object-oriented + # interface to the event + def instrument_internal_error(exception) + ActiveSupport::Notifications.instrument('internal_error.application', exception: exception) end end From d8c5055a811188505f9752fa6f090af39cdb2078 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Tue, 3 Sep 2024 13:24:01 +0100 Subject: [PATCH 07/11] build: add puma-metrics gem expose additional metrics for the puma server --- config/puma.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/config/puma.rb b/config/puma.rb index b53cc47..7bdabbf 100644 --- a/config/puma.rb +++ b/config/puma.rb @@ -38,6 +38,7 @@ # Allow puma to be restarted by `rails restart` command. plugin :tmp_restart +plugin :metrics # Use a custom log formatter to emit Puma log messages in a JSON format log_formatter do |str| From a77695608158ecfa7936b2ef58e360d47786adc6 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Tue, 3 Sep 2024 13:25:40 +0100 Subject: [PATCH 08/11] refactor: improved error control adds templated error pages to mirror other applications as well as controls 404 errors to be ignored in the `instrument_internal_error` metric --- app/controllers/application_controller.rb | 72 +++++++++++++++++++++-- config/routes.rb | 55 +---------------- public/landing/400.html | 27 +++++++++ public/landing/403.html | 26 ++++++++ public/landing/404.html | 26 ++++++++ public/landing/500.html | 27 +++++++++ 6 files changed, 175 insertions(+), 58 deletions(-) create mode 100644 public/landing/400.html create mode 100644 public/landing/403.html create mode 100644 public/landing/404.html create mode 100644 public/landing/500.html diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 12ac39e..36f271f 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -5,9 +5,7 @@ class ApplicationController < ActionController::Base # Prevent CSRF attacks by raising an exception. # For APIs, you may want to use :null_session instead. protect_from_forgery with: :exception - before_action :change_default_caching_policy - around_action :log_request_result # * Set cache control headers for HMLR apps to be public and cacheable @@ -25,7 +23,73 @@ def log_request_result detailed_request_log(duration) end - # rubocop:disable Metrics/MethodLength, Metrics/AbcSize + # Handle specific types of exceptions and render the appropriate error page + # 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| + case e.class + when ActionController::RoutingError, ActionView::MissingTemplate + :render404 + when ActionController::InvalidCrossOriginRequest + :render403 + when ActionController::ParameterMissing + :render400 + else + :handle_internal_error + end + end + end + + def handle_internal_error(exception) + # Notify subscribers of the internal error event and render the appropriate error page + # or attempt to render a generic error page if no specific error page exists + # unless the exception is a 404, in which case do nothing + instrument_internal_error(exception) unless exception.status == 404 + if exception.instance_of? ArgumentError + render_error(400) + else + Rails.logger.warn "No explicit error page for exception #{exception} - #{exception.class}" + render_error(500) + end + end + + def render_400(_exception = nil) # rubocop:disable Naming/VariableNumber + render_error(400) + end + + def render_403(_exception = nil) # rubocop:disable Naming/VariableNumber + render_error(403) + end + + def render_404(_exception = nil) # rubocop:disable Naming/VariableNumber + render_error(404) + end + + def render_500(_exception = nil) # rubocop:disable Naming/VariableNumber + render_error(500) + end + + def render_error(status) + reset_response + + respond_to do |format| + format.html { render_html_error_page(status) } + # Anything else returns the status as human readable plain string + format.all { render plain: Rack::Utils::HTTP_STATUS_CODES[status].to_s, status: status } + end + end + + def render_html_error_page(status) + render(layout: true, + file: Rails.root.join('public', 'landing', status.to_s), + status: status) + end + + def reset_response + self.response_body = nil + end + + # rubocop:disable Metrics/AbcSize, Metrics/MethodLength def detailed_request_log(duration) env = request.env @@ -53,7 +117,7 @@ def detailed_request_log(duration) Rails.logger.info(JSON.generate(log_fields)) end end - # rubocop:enable Metrics/MethodLength, Metrics/AbcSize + # rubocop:enable Metrics/AbcSize, Metrics/MethodLength # Notify subscriber(s) of an internal error event with the payload of the # exception once done diff --git a/config/routes.rb b/config/routes.rb index a32db39..74779a2 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -6,58 +6,5 @@ resource 'report-design', controller: :report_design, as: :report_design resource 'download-report', controller: :download_report, as: :download_report, only: [:show] - # 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 + get '*unmatched_route', to: 'application#render_404' end diff --git a/public/landing/400.html b/public/landing/400.html new file mode 100644 index 0000000..17d4430 --- /dev/null +++ b/public/landing/400.html @@ -0,0 +1,27 @@ + +
+
+
+

+ Request not understood +

+

+ We're sorry, but the request you made was not understood. If you are requesting data via a script + or program, please check the URL parameters. If you see this message as a result of using the HPI + or PPD applications, please let us know so that we can correct the problem. +

+

+ Who to contact +

+

+ If you are unable to access the data please fill in our contact form. +

+

+ For general transaction data enquiries email DRO@landregistry.gov.uk +

+

+ For general price paid data enquiries contact data.services@mail.landregistry.gov.uk. +

+
+
+
diff --git a/public/landing/403.html b/public/landing/403.html new file mode 100644 index 0000000..f4528c1 --- /dev/null +++ b/public/landing/403.html @@ -0,0 +1,26 @@ + +
+
+
+

+ Forbidden +

+

+ We're sorry, but it seems you don't have permission to access this resource. Please check the spelling + of the page address (URL). If you require further assistance, please see the contact details below. +

+

+ Who to contact +

+

+ If you are unable to access the data please fill in our contact form. +

+

+ For general transaction data enquiries email DRO@landregistry.gov.uk +

+

+ For general price paid data enquiries contact data.services@mail.landregistry.gov.uk. +

+
+
+
diff --git a/public/landing/404.html b/public/landing/404.html new file mode 100644 index 0000000..6769540 --- /dev/null +++ b/public/landing/404.html @@ -0,0 +1,26 @@ + +
+
+
+

+ Page not found +

+

+ We're sorry, but the web page you requested is not present on our server. Please check the spelling + of the page address (URL). If you require further assistance, please see the contact details below. +

+

+ Who to contact +

+

+ If you are unable to access the data please fill in our contact form. +

+

+ For general transaction data enquiries email DRO@landregistry.gov.uk +

+

+ For general price paid data enquiries contact data.services@mail.landregistry.gov.uk. +

+
+
+
diff --git a/public/landing/500.html b/public/landing/500.html new file mode 100644 index 0000000..4708d78 --- /dev/null +++ b/public/landing/500.html @@ -0,0 +1,27 @@ + +
+
+
+

+ Application error +

+

+ We're very sorry, but the request you just made resulted in an internal error in the application. + If this problem persists, please use one of the contact methods below to report the problem + to us, so that we can resolve it and help you get the data you require. +

+

+ Who to contact +

+

+ If you are unable to access the data please fill in our contact form. +

+

+ For general transaction data enquiries email DRO@landregistry.gov.uk +

+

+ For general price paid data enquiries contact data.services@mail.landregistry.gov.uk. +

+
+
+
From 3f983e8f05e47c91fa95f6a53fc4561e1d978b60 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Tue, 3 Sep 2024 13:42:36 +0100 Subject: [PATCH 09/11] build: Excluded prometheus metrics from the testing environment --- config.ru | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/config.ru b/config.ru index c713f67..a9e519f 100644 --- a/config.ru +++ b/config.ru @@ -4,11 +4,13 @@ require ::File.expand_path('config/environment', __dir__) -require 'prometheus/middleware/collector' -require 'prometheus/middleware/exporter' +unless Rails.env.test? + require 'prometheus/middleware/collector' + require 'prometheus/middleware/exporter' -use Prometheus::Middleware::Collector -use Prometheus::Middleware::Exporter + use Prometheus::Middleware::Collector + use Prometheus::Middleware::Exporter +end map Rails.application.config.relative_url_root || '/' do run Rails.application From baff4d34ac0b7ee1c16c4f40ae1d09113dc3f0f9 Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Tue, 3 Sep 2024 13:49:37 +0100 Subject: [PATCH 10/11] build: updated version patch cadence v1.5.3 ~> 1.5.4 --- 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 79e1666..e34760b 100644 --- a/app/lib/version.rb +++ b/app/lib/version.rb @@ -3,7 +3,7 @@ module Version MAJOR = 1 MINOR = 5 - PATCH = 3 + PATCH = 4 SUFFIX = nil VERSION = "#{MAJOR}.#{MINOR}.#{PATCH}#{SUFFIX && ".#{SUFFIX}"}" end From 633acaf782a0933ad5647024a9b6439ee923068e Mon Sep 17 00:00:00 2001 From: "Jon R. Humphrey" Date: Tue, 3 Sep 2024 13:49:52 +0100 Subject: [PATCH 11/11] docs: Updated CHANGELOG --- CHANGELOG.md | 70 +++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 56 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 88d6992..45efeff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,19 +1,60 @@ # Standard Reports UI: change log +## 1.5.4 - 2024-09 + +- (Jon) Implemented improved boilerplate metrics integration to offer analysis + of current application usage stats +- (Jon) Added `process_threads` gauge to prometheus metrics alongside isolating + thread counts to specific status' as per the Rails thread documentation + [GH-148](https://github.com/epimorphics/hmlr-linked-data/issues/148) +- (Jon) Updated the approach to resolve + [GH-142](https://github.com/epimorphics/hmlr-linked-data/issues/142) +- (Jon) Excluded prometheus metrics from the testing environment to reduce noise + in the logs +- (Jon) Added `puma-metrics` gem to provide base metrics for the Puma web server +- (Jon) Added the `internal_error_event` metric to the improved error controls +- (Jon) Tweaked the application controller to improve error control and display + provided message in themed pages to match the rest of the application +- (Jon) Added `puma.stats` to footer template in development environment only, + again as per the approach in the [`LR_Common_Styles` + gem](https://github.com/epimorphics/lr_common_styles/releases/tag/1.9.6) +- (Jon) Reorganised makefile targets alphabetically as well as mirrored other + improvements from the other applications in the suite +- (Jon) Updated .rubocop.yml primarily reorganising the rules alphabetically as + well as adding `CountAsOne` to both `Metrics/ClassLength` and + `Metrics/MethodLength`; includes files with removed earlier disabling of said + rules! +- (Jon) Updated `.gitignore` to include ignoring `byebug` history as well as + sets the tmp directory ignore to be anywhere, not just at the project root + ## 1.5.3 - 2024-08 -- (Dan) Update the error messages for the postcode selectors step 3/7 so each error message is unique to the postcode selector [120](https://github.com/epimorphics/standard-reports-ui/issues/120) -- (Dan) Updates alt text for screenshots of example reports [115](https://github.com/epimorphics/standard-reports-ui/issues/115) -- (Dan) Updates report page styles so links are underlined and gives download report page a seperate page title +- (Dan) Update the error messages for the postcode selectors step 3/7 so each + error message is unique to the postcode selector + [120](https://github.com/epimorphics/standard-reports-ui/issues/120) +- (Dan) Updates alt text for screenshots of example reports + [115](https://github.com/epimorphics/standard-reports-ui/issues/115) +- (Dan) Updates report page styles so links are underlined and gives download + report page a seperate page title - (Dan) Updates gemfile to use v1.9.5 lr_common_styles -- (Dan) Updates the page titles throughout the app [116](https://github.com/epimorphics/standard-reports-ui/issues/116) -- (Dan) Updates the button text on reports page to be dynamic and adds aria labels to help screen readers [119](https://github.com/epimorphics/standard-reports-ui/issues/119) -- (Dan) Update the error message for the postcode selectors step 3/7 [120](https://github.com/epimorphics/standard-reports-ui/issues/120) -- (Dan) Update the error message for the postcode selectors step 3/7 [120](https://github.com/epimorphics/standard-reports-ui/issues/120) -- (Dan) Updates step 3/7 to return user input rather than false when user inputs invalid value [118](https://github.com/epimorphics/standard-reports-ui/issues/118) -- (Dan) Adds underline text to laning page of standard reports and to help link[114](https://github.com/epimorphics/standard-reports-ui/issues/114) -- (Dan) Styled the help button to match PPD [117](https://github.com/epimorphics/standard-reports-ui/issues/117) -- (Dan) Adds more descriptive text to action buttons on the report page [115](https://github.com/epimorphics/standard-reports-ui/issues/115) +- (Dan) Updates the page titles throughout the app + [116](https://github.com/epimorphics/standard-reports-ui/issues/116) +- (Dan) Updates the button text on reports page to be dynamic and adds aria + labels to help screen readers + [119](https://github.com/epimorphics/standard-reports-ui/issues/119) +- (Dan) Update the error message for the postcode selectors step 3/7 + [120](https://github.com/epimorphics/standard-reports-ui/issues/120) +- (Dan) Update the error message for the postcode selectors step 3/7 + [120](https://github.com/epimorphics/standard-reports-ui/issues/120) +- (Dan) Updates step 3/7 to return user input rather than false when user inputs + invalid value + [118](https://github.com/epimorphics/standard-reports-ui/issues/118) +- (Dan) Adds underline text to laning page of standard reports and to help + link[114](https://github.com/epimorphics/standard-reports-ui/issues/114) +- (Dan) Styled the help button to match PPD + [117](https://github.com/epimorphics/standard-reports-ui/issues/117) +- (Dan) Adds more descriptive text to action buttons on the report page + [115](https://github.com/epimorphics/standard-reports-ui/issues/115) ## 1.5.2 - 2023-11-27 @@ -29,7 +70,8 @@ - (Jon) Updated the `app/controllers/application_controller.rb` to include the `before_action` for the `change_default_caching_policy` method to ensure the - default `Cache-Control` header for all requests is set to 5 minutes (300 seconds). + default `Cache-Control` header for all requests is set to 5 minutes (300 + seconds). ## 1.5.0 - 2023-07-05 @@ -43,8 +85,8 @@ - (Jon) Keeping with the logging improvements this change rewrites the messages passed to the logging gem to ensure the included details are simple and straight to the point. This should also improve the issues reported in - [GH-117](https://github.com/epimorphics/hmlr-linked-data/issues/117) , - at least from the front-end point of view! + [GH-117](https://github.com/epimorphics/hmlr-linked-data/issues/117) , at + least from the front-end point of view! - (Jon) Updated the [README](README.md) to include the `API_SERVICE_URL` variable to ensure the local `standard_reports_manager` instance is used in the test environment.