Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Patch: Hotfix backport #256

Merged
merged 21 commits into from
Oct 11, 2024
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
f4b63d5
Merge pull request #248 from epimorphics/hotfix/release-v1.7.8
jonrandahl Sep 17, 2024
bed09fc
Merge pull request #252 from epimorphics/preprod
bogdanadrianmarc Oct 1, 2024
1e94f78
refactor: adjusted internal error instrumentation
jonrandahl Oct 8, 2024
4b0f029
refactor: improved error reporting
jonrandahl Oct 8, 2024
9537458
refactor: updated logging in error control
jonrandahl Oct 8, 2024
87721f4
build: updated Makefile targets
jonrandahl Oct 8, 2024
2f739e4
build: increment version patch cadence
jonrandahl Oct 8, 2024
c9427b0
docs: updated CHANGELOG
jonrandahl Oct 8, 2024
2313a8c
refactor: reduced complexity for method argument
jonrandahl Oct 9, 2024
7e94a85
refactor: additional reductions in complexity
jonrandahl Oct 9, 2024
bdfec9e
Merge pull request #255 from epimorphics/hotfix/reduce-log-noise
jonrandahl Oct 9, 2024
d9dd116
Merge branch 'hotfix/reduce-log-noise' into patch/backport-log-noise-…
jonrandahl Oct 9, 2024
e478611
fix: remove introduced `:not_found` catch
jonrandahl Oct 9, 2024
ba13cd0
build: remove erroneous `bundle config` from `assets` target
jonrandahl Oct 9, 2024
ea46db3
test: fixed failing test for results summary comparison
jonrandahl Oct 9, 2024
af41a1e
test: regenerated VCR Cassette
jonrandahl Oct 9, 2024
ea46dd3
build: update to bundler version record
jonrandahl Oct 9, 2024
f60dcb2
build: removed erroneous test target command
jonrandahl Oct 9, 2024
234f4d1
build: incremented version patch cadence
jonrandahl Oct 9, 2024
353d229
docs: updated CHANGELOG
jonrandahl Oct 9, 2024
da6bef9
Merge branch 'spike/resolve-search-error' into patch/backport-hotfix-…
jonrandahl Oct 9, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,21 @@ This app allows the user to explore HMLR price-paid open linked data.

## Changelog

## 1.7.10 - 2024-10

- (Jon) Updated the message reported to the error page while in development mode
to include the error message and the status code; also configured the helper
to display the actual rails stack trace in development mode
- (Jon) Added the `log_error` helper to the `render_error_page` helper method in
the `search_controller` to apply the appropriate log level based on the status
code
- (Jon) Added `RoutingError` and `MissingTemplate` to the list of exceptions to
be caught by the `render_error_page` helper method in the `search_controller`
- (Jon) Wrapped the Internal Error Instrumentation in an `unless` block to
ensure the application does not report internal errors to the Prometheus
metrics when the error is a 404 or 422 thereby reducing the noise in the Slack
alerts channel

## 1.7.9 - 2024-09

- (Jon) Updated the application exceptions controller to instrument the
Expand Down
8 changes: 3 additions & 5 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,8 @@ lint: assets
@./bin/bundle exec rubocop

local:
@echo "Installing all packages ..."
@./bin/bundle install
@echo "Starting local server ..."
@./bin/rails server -p ${PORT}
@API_SERVICE_URL=${API_SERVICE_URL} ./bin/rails server -p ${PORT}

publish: image
@echo Publishing image: ${REPO}:${TAG} ...
Expand All @@ -98,8 +96,8 @@ tag:
@echo ${TAG}

test: assets
@echo "Running unit tests ..."
@./bin/rails test:unit
@echo "Running tests ..."
@./bin/rails test
@echo "Running system tests ..."
@./bin/rails test:system

Expand Down
14 changes: 8 additions & 6 deletions app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,18 @@ def log_request_result
# or attempt to render a generic error page if no specific error page exists
unless Rails.application.config.consider_all_requests_local
rescue_from StandardError do |e|
# Instrument ActiveSupport::Notifications for internal errors:
ActiveSupport::Notifications.instrument('internal_error.application', exception: e)
# Instrument ActiveSupport::Notifications for internal errors but only for non-404 errors:
unless e.is_a?(ActionController::RoutingError) || e.is_a?(ActionView::MissingTemplate)
ActiveSupport::Notifications.instrument('internal_error.application', exception: e)
end

# Trigger the appropriate error handling method based on the exception
case e.class
when ActiveRecord::RecordNotFound, ActionController::RoutingError, ActionView::MissingTemplate
when ActionController::RoutingError, ActionView::MissingTemplate
:render404
when ActionController::InvalidCrossOriginRequest
:render403
when ActiveRecord::RecordInvalid, ActionController::ParameterMissing
when ActionController::BadRequest, ActionController::ParameterMissing
:render400
else
:handle_internal_error
Expand Down Expand Up @@ -101,7 +104,6 @@ def reset_response
# rubocop:disable Metrics/AbcSize, Metrics/MethodLength
def detailed_request_log(duration)
env = request.env

log_fields = {
duration: duration,
request_id: env['X_REQUEST_ID'],
Expand All @@ -113,7 +115,7 @@ 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
Expand Down
32 changes: 26 additions & 6 deletions app/controllers/search_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ def index
create
end

def create # rubocop:disable Metrics/AbcSize, Metrics/MethodLength, Metrics/PerceivedComplexity
def create # rubocop:disable Metrics/MethodLength
@preferences = UserPreferences.new(params)

if @preferences.empty?
Expand All @@ -25,15 +25,22 @@ def create # rubocop:disable Metrics/AbcSize, Metrics/MethodLength, Metrics/Perc
end
rescue StandardError => e
e = e.cause || e
rescue_standard_error(e)
end

status = case e
# Determine status symbol to pass to the error page
def rescue_standard_error(err)
status = case err
when RoutingError, MissingTemplate
:not_found
when MalformedSearchError, ArgumentError
:bad_request
else
:internal_server_error
end

render_error_page(e, e.message, status) if !Rails.env.development?
# Display the actual rails error stack trace while in development
render_error_page(err, err.message, status) unless Rails.env.development?
# To check the error page in development, set the RAILS_ENV to production or test
end

def use_compact_json?
Expand All @@ -54,10 +61,10 @@ def render_error_page(err, message, status, template = 'ppd/error')
@message = message

# log the error with as much detail as possible in development to aid in resolving the issue
@message = "#{err.class.name} error: #{message}" if Rails.env.development?
@message = "#{Rack::Utils::SYMBOL_TO_STATUS_CODE[status]} ~ #{err.class.name} error: #{message}" if Rails.env.development?

# Keep it simple silly in production!
Rails.logger.error message
log_error(Rack::Utils::SYMBOL_TO_STATUS_CODE[status], message)

@error_message =
[
Expand All @@ -68,4 +75,17 @@ def render_error_page(err, message, status, template = 'ppd/error')
render(template: template, status: status)
end
# rubocop:enable Layout/LineLength, Metrics/MethodLength

# Log the error with the appropriate log level based on the status code
def log_error(status, message)
case status
when 500..599
Rails.logger.error(message)
when 400..499
Rails.logger.warn(message)
else
Rails.logger.info(message)
end
end

end
2 changes: 1 addition & 1 deletion app/lib/version.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
module Version
MAJOR = 1
MINOR = 7
PATCH = 9
PATCH = 10
SUFFIX = nil
VERSION = "#{MAJOR}.#{MINOR}.#{PATCH}#{SUFFIX && ".#{SUFFIX}"}"
end