From 62c25161d2acf5e6ec64af81bfa38118d0410c93 Mon Sep 17 00:00:00 2001 From: Daniel Schmidt Date: Fri, 10 Jul 2026 09:46:46 -0700 Subject: [PATCH] 1.x performance enhancements - DB / Solr - Support DB pool sizing via `ENV.DATABASE_POOL`. - Restrict search facets to items configurable in the sidebar. - Health checks - Decrease health check timeouts to `0.9s`. - Skip health checks for which there's no URL. - Refactors health checks to traditional Ruby module layout. - Bots and the /track endpoint - Blocks `/catalog/*/track` in robots.txt. - Adds no-op `GET /catalog/:id/track` route to cut down on log noise. --- app/controllers/catalog_controller.rb | 28 ++++++- config/database.yml | 7 +- config/initializers/okcomputer.rb | 33 +++++--- config/routes.rb | 1 + docker-compose.yml | 10 ++- lib/geo_data_health_check.rb | 6 ++ .../connection_failed_error.rb | 3 + lib/geo_data_health_check/http_head_check.rb | 84 +++++++++++++++++++ .../timed_health_check.rb | 42 ++++++++++ lib/http_head_check.rb | 54 ------------ lib/templates/robots.txt.erb | 3 +- spec/lib/http_head_check_spec.rb | 2 +- spec/lib/timed_health_check_spec.rb | 67 +++++++++++++++ spec/models/search_builder_spec.rb | 10 +++ spec/requests/catalog_tracking_spec.rb | 17 ++++ spec/requests/okcomputer_spec.rb | 13 ++- 16 files changed, 300 insertions(+), 80 deletions(-) create mode 100644 lib/geo_data_health_check.rb create mode 100644 lib/geo_data_health_check/connection_failed_error.rb create mode 100644 lib/geo_data_health_check/http_head_check.rb create mode 100644 lib/geo_data_health_check/timed_health_check.rb delete mode 100644 lib/http_head_check.rb create mode 100644 spec/lib/timed_health_check_spec.rb create mode 100644 spec/requests/catalog_tracking_spec.rb diff --git a/app/controllers/catalog_controller.rb b/app/controllers/catalog_controller.rb index 21c5b8d..4502fca 100644 --- a/app/controllers/catalog_controller.rb +++ b/app/controllers/catalog_controller.rb @@ -4,6 +4,21 @@ class CatalogController < ApplicationController include Blacklight::Catalog + SEARCH_FACET_FIELDS = [ + Settings.FIELDS.INDEX_YEAR, + Settings.FIELDS.SPATIAL_COVERAGE, + Settings.FIELDS.ACCESS_RIGHTS, + Settings.FIELDS.RESOURCE_CLASS, + Settings.FIELDS.RESOURCE_TYPE, + Settings.FIELDS.FORMAT, + Settings.FIELDS.SUBJECT, + Settings.FIELDS.THEME, + Settings.FIELDS.CREATOR, + Settings.FIELDS.PUBLISHER, + Settings.FIELDS.PROVIDER, + Settings.FIELDS.GEOREFERENCED + ].freeze + configure_blacklight do |config| # Ensures that JSON representations of Solr Documents can be retrieved using @@ -117,10 +132,10 @@ class CatalogController < ApplicationController config.add_facet_field Settings.FIELDS.SOURCE, label: 'Source', show: false config.add_facet_field Settings.FIELDS.VERSION, label: 'Is Version Of', show: false - # Have BL send all facet field names to Solr, which has been the default - # previously. Simply remove these lines if you'd rather use Solr request - # handler defaults, or have no facets. - config.add_facet_fields_to_solr_request! + # Only request facets displayed in the sidebar. The remaining configured + # facets are internal filter fields; faceting them on every search is + # expensive on production-sized Solr indexes, especially locn_geometry. + config.add_facet_fields_to_solr_request!(*SEARCH_FACET_FIELDS) # SEARCH RESULTS FIELDS @@ -287,4 +302,9 @@ def web_services end end end + + def track_no_content + response.set_header('X-Robots-Tag', 'noindex, nofollow') + head :no_content + end end diff --git a/config/database.yml b/config/database.yml index d31698f..a18ddfd 100644 --- a/config/database.yml +++ b/config/database.yml @@ -1,10 +1,13 @@ --- development: - url: postgres://root:root@db/geodata-dev?pool=5 + url: <%= ENV['DATABASE_URL'] || 'postgres://root:root@db/geodata-dev' %> + pool: <%= ENV.fetch('DATABASE_POOL', ENV.fetch('RAILS_MAX_THREADS', 5)) %> test: - url: postgres://root:root@db/geodata-test?pool=5 + url: <%= ENV['DATABASE_URL'] || 'postgres://root:root@db/geodata-test' %> + pool: <%= ENV.fetch('DATABASE_POOL', ENV.fetch('RAILS_MAX_THREADS', 5)) %> production: url: <%= ENV['DATABASE_URL'] || 'postgres://root:root@db/geodata-prod' %> + pool: <%= ENV.fetch('DATABASE_POOL', ENV.fetch('RAILS_MAX_THREADS', 5)) %> diff --git a/config/initializers/okcomputer.rb b/config/initializers/okcomputer.rb index b4a35b3..83ae014 100644 --- a/config/initializers/okcomputer.rb +++ b/config/initializers/okcomputer.rb @@ -1,27 +1,34 @@ # initializers/okcomputer.rb # Health checks configuration -require_relative '../../lib/http_head_check' +require_relative '../../lib/geo_data_health_check' OkComputer.logger = Rails.logger OkComputer.check_in_parallel = true +health_check_timeout = 0.9 + +timed_health_check = ->(check) do + GeoDataHealthCheck::TimedHealthCheck.new(check, timeout: health_check_timeout) +end + +OkComputer::Registry.register 'default', timed_health_check.call(OkComputer::Registry.fetch('default')) +OkComputer::Registry.register 'database', timed_health_check.call(OkComputer::Registry.fetch('database')) + # Check that DB migrations have run -OkComputer::Registry.register 'database-migrations', OkComputer::ActiveRecordMigrationsCheck.new +OkComputer::Registry.register 'database-migrations', timed_health_check.call(OkComputer::ActiveRecordMigrationsCheck.new) # Check the Solr connection # Requires the ping handler on the solr core (/admin/ping). core_baseurl = Blacklight.default_index.connection.uri.to_s.chomp('/') -OkComputer::Registry.register 'solr', OkComputer::SolrCheck.new(core_baseurl) - -# Perform a Head request to check geoserver endpoint -geoserver_url = Rails.configuration.x.servers[:geoserver] -OkComputer::Registry.register 'geoserver', GeoDataHealthCheck::HttpHeadCheck.new(geoserver_url) +OkComputer::Registry.register 'solr', timed_health_check.call(OkComputer::SolrCheck.new(core_baseurl, 1)) -# Perform a Head request to check secure_geoserver endpoint -geoserver_secure_url = Rails.configuration.x.servers[:geoserver_secure] -OkComputer::Registry.register 'geoserver_secure', GeoDataHealthCheck::HttpHeadCheck.new(geoserver_secure_url) +{ + geoserver: Rails.configuration.x.servers[:geoserver], + geoserver_secure: Rails.configuration.x.servers[:geoserver_secure], + spatial_server: Rails.configuration.x.servers[:spatial_server] +}.each do |name, url| + next if url.blank? -# Perform a Head request to check spatial server endpoint -spatial_server_url = Rails.configuration.x.servers[:spatial_server] -OkComputer::Registry.register 'spatial_server', GeoDataHealthCheck::HttpHeadCheck.new(spatial_server_url) + OkComputer::Registry.register name.to_s, timed_health_check.call(GeoDataHealthCheck::HttpHeadCheck.new(url, health_check_timeout)) +end diff --git a/config/routes.rb b/config/routes.rb index eed3e14..069d38f 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -7,6 +7,7 @@ resource :catalog, only: [:index], as: 'catalog', path: '/catalog', controller: 'catalog' do concerns :searchable + get ':id/track', action: :track_no_content, as: :track_no_content end devise_for :users, controllers: { omniauth_callbacks: 'omniauth_callbacks' } diff --git a/docker-compose.yml b/docker-compose.yml index 7e6fefe..3d56136 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -14,7 +14,15 @@ services: stop_signal: SIGQUIT stop_grace_period: 15s environment: - - DATABASE_URL=postgres://root:root@db/geodata-dev?pool=5 + - RAILS_ENV=${RAILS_ENV:-production} + - RAILS_LOG_TO_STDOUT=${RAILS_LOG_TO_STDOUT:-true} + - RAILS_SERVE_STATIC_FILES=${RAILS_SERVE_STATIC_FILES:-true} + - SECRET_KEY_BASE=${SECRET_KEY_BASE:-development-secret-key-base} + - MIN_THREADS=${MIN_THREADS:-1} + - RAILS_MAX_THREADS=${RAILS_MAX_THREADS:-5} + - PUMA_WORKERS=${PUMA_WORKERS:-2} + - DATABASE_POOL=${DATABASE_POOL:-${RAILS_MAX_THREADS:-5}} + - DATABASE_URL=postgres://root:root@db/geodata-dev - SOLR_URL=http://solr:8983/solr/geodata-test volumes: - ./:/opt/app:delegated diff --git a/lib/geo_data_health_check.rb b/lib/geo_data_health_check.rb new file mode 100644 index 0000000..8646850 --- /dev/null +++ b/lib/geo_data_health_check.rb @@ -0,0 +1,6 @@ +require_relative 'geo_data_health_check/connection_failed_error' +require_relative 'geo_data_health_check/http_head_check' +require_relative 'geo_data_health_check/timed_health_check' + +module GeoDataHealthCheck +end diff --git a/lib/geo_data_health_check/connection_failed_error.rb b/lib/geo_data_health_check/connection_failed_error.rb new file mode 100644 index 0000000..caa824a --- /dev/null +++ b/lib/geo_data_health_check/connection_failed_error.rb @@ -0,0 +1,3 @@ +module GeoDataHealthCheck + class ConnectionFailedError < StandardError; end +end diff --git a/lib/geo_data_health_check/http_head_check.rb b/lib/geo_data_health_check/http_head_check.rb new file mode 100644 index 0000000..35336ca --- /dev/null +++ b/lib/geo_data_health_check/http_head_check.rb @@ -0,0 +1,84 @@ +require 'net/http' +require 'openssl' +require 'uri' + +require_relative 'connection_failed_error' + +module GeoDataHealthCheck + class HttpHeadCheck < OkComputer::Check + DEFAULT_REQUEST_TIMEOUT = 5 + SUCCESS_MESSAGE = 'Http head check successful.'.freeze + SUCCESSFUL_RESPONSE_CLASSES = [ + Net::HTTPOK, + Net::HTTPRedirection + ].freeze + + attr_reader :url, :request_timeout + + def initialize(url, request_timeout = DEFAULT_REQUEST_TIMEOUT) + super() + @url = url + @request_timeout = request_timeout + end + + def check + response = perform_request + + if successful_response?(response) + mark_message SUCCESS_MESSAGE + else + mark_failure + mark_message unexpected_status_message(response) + end + rescue StandardError => e + mark_message "Error: '#{e.message}'" + mark_failure + end + + def perform_request + head_request + rescue Net::OpenTimeout, Net::ReadTimeout => e + raise ConnectionFailedError, "#{url} did not respond within #{request_timeout} seconds: #{e.message}" + rescue ArgumentError => e + raise ConnectionFailedError, "Invalid URL format for '#{url}': #{e.class}: #{e.message}" + rescue StandardError => e + raise ConnectionFailedError, e.message + end + + private + + def head_request + uri = parsed_uri + + Net::HTTP.start( + uri.host, + uri.port, + use_ssl: uri.is_a?(URI::HTTPS), + verify_mode: OpenSSL::SSL::VERIFY_PEER, + open_timeout: request_timeout, + read_timeout: request_timeout + ) do |http| + http.head(uri.request_uri) + end + end + + def parsed_uri + URI.parse(url).tap do |uri| + raise ArgumentError, 'URL must include an HTTP or HTTPS scheme and host' unless http_uri?(uri) + end + end + + def http_uri?(uri) + uri.is_a?(URI::HTTP) && uri.host + end + + def successful_response?(response) + SUCCESSFUL_RESPONSE_CLASSES.any? { |klass| response.is_a?(klass) } + end + + def unexpected_status_message(response) + "Error: '#{url}' http head check responded, but returned unexpected HTTP status: " \ + "#{response.code} #{response.class}. Expected 200 Net::HTTPOK or a redirect." + end + end +end diff --git a/lib/geo_data_health_check/timed_health_check.rb b/lib/geo_data_health_check/timed_health_check.rb new file mode 100644 index 0000000..a450d50 --- /dev/null +++ b/lib/geo_data_health_check/timed_health_check.rb @@ -0,0 +1,42 @@ +require 'timeout' + +module GeoDataHealthCheck + class TimedHealthCheck < OkComputer::Check + attr_reader :wrapped_check, :timeout + + def initialize(wrapped_check, timeout:) + super() + @wrapped_check = wrapped_check + @timeout = timeout + end + + def registrant_name=(name) + super + wrapped_check.registrant_name = name if wrapped_check.respond_to?(:registrant_name=) + end + + def check + Timeout.timeout(timeout) do + wrapped_check.run + end + + copy_wrapped_check_result + rescue Timeout::Error + fail_with_message "Health check timed out after #{timeout} seconds" + rescue StandardError => e + fail_with_message "Error: '#{e.message}'" + end + + private + + def copy_wrapped_check_result + mark_message wrapped_check.message + mark_failure unless wrapped_check.success? + end + + def fail_with_message(message) + mark_failure + mark_message message + end + end +end diff --git a/lib/http_head_check.rb b/lib/http_head_check.rb deleted file mode 100644 index a3b74a0..0000000 --- a/lib/http_head_check.rb +++ /dev/null @@ -1,54 +0,0 @@ -module GeoDataHealthCheck - class HttpHeadCheck < OkComputer::Check - attr_accessor :url, :request_timeout - - # rubocop:disable Lint/MissingSuper - def initialize(url, request_timeout = 5) - self.url = url - self.request_timeout = request_timeout - end - # rubocop:enable Lint/MissingSuper - - def check - response = perform_request - - if response.is_a?(Net::HTTPOK) || response.is_a?(Net::HTTPRedirection) - mark_message 'Http head check successful.' - else - mark_message "Error: '#{url}' http head check responded, but returned unexpeced HTTP status: #{response.code} #{response.class}. Expected 200 Net::HTTPOK." - mark_failure - end - rescue StandardError => e - mark_message "Error: '#{e.message}'" - mark_failure - end - - def perform_request - head_request - rescue Net::OpenTimeout, Net::ReadTimeout => e - raise ConnectionFailedError, "#{url} did not respond within #{request_timeout} seconds: #{e.message}" - rescue ArgumentError => e - raise ConnectionFailedError, "Invalid URL format for '#{url}': #{e.class}: #{e.message}" - rescue StandardError => e - raise ConnectionFailedError, e.message - end - - private - - def head_request - uri = URI(url) - Net::HTTP.start( - uri.host, - uri.port, - use_ssl: uri.scheme == 'https', - verify_mode: OpenSSL::SSL::VERIFY_PEER, - open_timeout: request_timeout, - read_timeout: request_timeout - ) do |http| - http.head(uri.request_uri) - end - end - end - - class ConnectionFailedError < StandardError; end -end diff --git a/lib/templates/robots.txt.erb b/lib/templates/robots.txt.erb index 5316d5c..3241a52 100644 --- a/lib/templates/robots.txt.erb +++ b/lib/templates/robots.txt.erb @@ -18,6 +18,7 @@ Disallow: /catalog.rss Disallow: /catalog*f[ Disallow: /catalog*f%5B Disallow: /catalog/*/relations +Disallow: /catalog/*/track Disallow: /catalog/facet/* Disallow: /catalog/*/web_services Disallow: /catalog/email @@ -50,4 +51,4 @@ Disallow: / User-agent: DataForSeoBot Disallow: / -Sitemap: <%= @base_url %>/sitemap.xml.gz \ No newline at end of file +Sitemap: <%= @base_url %>/sitemap.xml.gz diff --git a/spec/lib/http_head_check_spec.rb b/spec/lib/http_head_check_spec.rb index baac553..6fdf414 100644 --- a/spec/lib/http_head_check_spec.rb +++ b/spec/lib/http_head_check_spec.rb @@ -1,5 +1,5 @@ require 'rails_helper' -require_relative '../../lib/http_head_check' +require_relative '../../lib/geo_data_health_check' RSpec.describe GeoDataHealthCheck::HttpHeadCheck do let(:url) { 'https://example.com/endpoint' } diff --git a/spec/lib/timed_health_check_spec.rb b/spec/lib/timed_health_check_spec.rb new file mode 100644 index 0000000..1c4b2b5 --- /dev/null +++ b/spec/lib/timed_health_check_spec.rb @@ -0,0 +1,67 @@ +require 'rails_helper' +require_relative '../../lib/geo_data_health_check' + +RSpec.describe GeoDataHealthCheck::TimedHealthCheck do + let(:passing_health_check_class) do + Class.new(OkComputer::Check) do + def check + mark_message 'ok' + end + end + end + + let(:failing_health_check_class) do + Class.new(OkComputer::Check) do + def check + mark_failure + mark_message 'failed' + end + end + end + + let(:slow_health_check_class) do + Class.new(OkComputer::Check) do + def check + sleep 0.2 + mark_message 'too late' + end + end + end + + it 'preserves a passing wrapped check result' do + check = described_class.new(passing_health_check_class.new, timeout: 0.1) + + check.run + + expect(check).to be_success + expect(check.message).to eq('ok') + end + + it 'preserves a failing wrapped check result' do + check = described_class.new(failing_health_check_class.new, timeout: 0.1) + + check.run + + expect(check).not_to be_success + expect(check.message).to eq('failed') + end + + it 'fails when the wrapped check exceeds the timeout' do + check = described_class.new(slow_health_check_class.new, timeout: 0.01) + + check.run + + expect(check).not_to be_success + expect(check.message).to eq('Health check timed out after 0.01 seconds') + expect(check.time).to be < 0.1 + end + + it 'passes the registered check name to the wrapped check' do + wrapped_check = passing_health_check_class.new + check = described_class.new(wrapped_check, timeout: 0.1) + + check.registrant_name = 'database' + + expect(wrapped_check.registrant_name).to eq('database') + end +end diff --git a/spec/models/search_builder_spec.rb b/spec/models/search_builder_spec.rb index 2d8031c..566b2dc 100644 --- a/spec/models/search_builder_spec.rb +++ b/spec/models/search_builder_spec.rb @@ -10,4 +10,14 @@ expect(SearchBuilder.included_modules).to include(Geoblacklight::SuppressedRecordsSearchBehavior) end end + + describe 'facet requests' do + it 'does not ask Solr to facet internal filter-only fields on every search' do + facet_fields = CatalogController.blacklight_config.facet_fields + + expect(facet_fields[Settings.FIELDS.GEOMETRY].include_in_request).not_to be true + expect(facet_fields[Settings.FIELDS.MEMBER_OF].include_in_request).not_to be true + expect(facet_fields[Settings.FIELDS.SPATIAL_COVERAGE].include_in_request).to be true + end + end end diff --git a/spec/requests/catalog_tracking_spec.rb b/spec/requests/catalog_tracking_spec.rb new file mode 100644 index 0000000..5017fb7 --- /dev/null +++ b/spec/requests/catalog_tracking_spec.rb @@ -0,0 +1,17 @@ +require 'rails_helper' + +RSpec.describe 'Catalog tracking', type: :request do + it 'returns no content for GET requests to Blacklight tracking URLs' do + get '/catalog/stanford-bh565zz1424/track' + + expect(response).to have_http_status(:no_content) + expect(response.headers['X-Robots-Tag']).to eq('noindex, nofollow') + end + + it 'keeps POST requests routed to Blacklight search tracking' do + post '/catalog/stanford-bh565zz1424/track', params: { counter: 2, document_id: 'stanford-bh565zz1424' } + + expect(response).to have_http_status(:see_other) + expect(response).to redirect_to('/catalog/stanford-bh565zz1424') + end +end diff --git a/spec/requests/okcomputer_spec.rb b/spec/requests/okcomputer_spec.rb index 7801eca..0a56f79 100644 --- a/spec/requests/okcomputer_spec.rb +++ b/spec/requests/okcomputer_spec.rb @@ -8,15 +8,20 @@ it 'returns all checks at /health' do get '/health' - expect(response).to have_http_status :internal_server_error + expect(response).to have_http_status :ok expect(response.parsed_body.keys).to match_array %w[ default database database-migrations solr - geoserver - geoserver_secure - spatial_server ] end + + it 'keeps every /health check under one second' do + get '/health' + + expect(response.parsed_body.values).to all( + include('time' => be < 1.0) + ) + end end