Release 2.0.0: stable exception contract, observable retries, dead-code and bug removal - #43
Release 2.0.0: stable exception contract, observable retries, dead-code and bug removal#43joescottdave wants to merge 9 commits into
Conversation
… events Service previously logged directly to Rails.logger (with no way for consuming apps to disable, reformat, or change level) and enabled Faraday's debug-level request/response logging unconditionally. It also depended on yajl-ruby to re-parse JSON response bodies that Faraday's own :json middleware had already parsed, which was both redundant and broken out of the box (the gem's own `require "yajl"` was commented out). - Remove all logger:/log_message/generate_service_message/Rails.logger usage; consumers now subscribe to instrumentation events instead - Namespace all notification event names under `data_services_api` (was the generic, collision-prone `.api` suffix), and add a new query_result event carrying path/method/status/returned_rows - Make Faraday's own logging middleware opt-in via faraday_logger:/ faraday_logger_options:, defaulting to debug level when enabled - Drop the yajl-ruby dependency and the parse_json/report_json_failure code built on it, since Faraday already parses response bodies - Fix a NameError (RACK::Exception typo) in the service-exception path BREAKING CHANGE: the `logger:` config option is removed, and the response.api/connection_failure.api/service_exception.api/requests.api notification names are renamed to their data_services_api-suffixed equivalents. Consuming apps must subscribe to the new event names and provide their own logging via ActiveSupport::Notifications subscribers.
Auditing service.rb turned up several methods that were broken or unreachable but had gone unnoticed because no consuming app exercised them: - Service#datasets always raised ArgumentError (missing required argument to api_get_json) - as_http_api raised URI::InvalidComponentError whenever url: was configured with a scheme, exactly as the README's own example shows, because URI::HTTP.build(host: @url, ...) treated the full URL as a bare hostname - Service#ok? was unreachable (Faraday's raise_error middleware already raises on all 4xx/5xx first) and would have raised a TypeError itself if it ever ran, since response.body is already a parsed Hash by then - create_http_connection's auth parameter was unused and non-functional (referenced api_user/api_pw methods that don't exist) Also drop the unused faraday-encoding dependency, add a connection_timeout config option in place of the hardcoded 600s default, and extract the duplicated request-timing/instrumentation logic in get_from_api/post_to_api into a shared perform_request helper.
… hook Faraday's own exception types (ResourceNotFound, ClientError, ServerError, ParsingError) were leaking straight through to callers for anything beyond a 404. But both consuming apps (ppd-explorer, ukhpi) rescue DataServicesApi::ServiceException and call e.service_message on it, expecting this gem to own that contract rather than exposing Faraday's exception hierarchy directly. - perform_request now catches the full Faraday::Error hierarchy and re-raises as ServiceException for any 4xx/5xx status or unparseable body; network-level failures (TimeoutError/ConnectionFailed) are left as Faraday's own types, since they're transport failures, not API ones - Fixed ServiceException#service_message, which always returned nil due to a typo (@service_msg instead of @service_message) - the exact accessor both consuming apps already call on rescue - service_exception.data_services_api now fires for this whole class of failure instead of just 404s, and its query_string field is populated from the actual request params instead of always being nil - Added a retry.data_services_api notification, fired before each retry attempt on a network failure, since retries were previously invisible to any instrumentation subscriber README and CHANGELOG updated to document the exception contract and the new notification.
Service#dataset(name) only ever populated data-api/dataset in the JSON handed to Dataset, never structure-api/describe-api, so any Dataset obtained the normal way (the only way any consuming app gets one) had those fields as nil. Dataset#structure then compounded it by calling api_get_json with a missing required argument - the same class of bug already found and fixed in Service#datasets. - Derive structure-api/describe-api from data-api the same way the real /dataset listing endpoint returns them (<data-api>/structure, <data-api>/describe), confirmed against old cassette fixtures - Fix Dataset#structure's missing argument to api_get_json - Add test coverage for Dataset#structure and Dataset#describe
… emitted by removing in favour of the more generic
joescottdave
changed the title
|
Wondering if we should mark unused methods with bugs (fixed here) as deprecated with a view to removing them entirely. |
|
I'm gradually reviewing this. It looks to be fine, though I'm stepping through things to figure out what is going on, so it is taking time. I've added some Ruby type signatures (rbs files) to a local branch and am using The other thing I'm looking at is the VCRs, some of which have been updated, but others which date back to 2017. If I remove them and try to rebuild them against a local API running against the live SPARQL endpoint, I get some 404s, which may just be that the test instances need updating. |
1st error is indeed the 2nd error is 3rd error is
As this is a major version bump, I'd vote to remove these methods and tests. |
2 participants
Objective
The amount of time it takes to make very small (and, unfortunately, error-prone) tweaks
to the logging in the HMLR suite of applications is incredibly high, in part because we
first have to update gems like this one and then adopt the new version across four
different apps and deploy them to three different environments.
During an investigation into a silently crashing log-line (replicated in three of these
apps) we find that this gem is contributing to the noise with a misconfiguration of
Faradaythat cannot be altered from the outside, hence it is once again necessaryto make a fix here and to update the gem in two places
(epimorphics/standard-reports-ui, and epimorphics/ppd-explorer).
The gem, until now, has accepted the
Rails.loggerinstance at initialisation and usesit to print logs of its own. In
v2the gem will switch to theActiveSupport::Notificationand
ActiveSupport::Subscriberpattern so that consuming applications can log as theychoose and we reduce future need to revisit this gem any time there is a problem with
the log quality.
Breaking changes
logger:config, no automaticRails.loggerwiring, no debug-level Faraday logging on by default. Thegem now only emits
ActiveSupport::Notificationsevents; consuming appssubscribe and log however they want. Faraday's own request/response
logging is opt-in via
faraday_logger:..apisuffix onto.data_services_api(
response.api->response.data_services_api, etc).status or unparseable body is now always wrapped in
DataServicesApi::ServiceExceptionbefore reaching the caller. Thisrestores the exception contract consuming apps were already written
against (
rescue DataServicesApi::ServiceException,e.service_message)but weren't reliably getting, since only 404s were ever wrapped before,
and even that path was largely dead code.
New
retry.data_services_api: fired before each retry attempt on a networkfailure, so retry behaviour is observable for the first time instead of
happening silently inside Faraday.
connection_timeoutconfig option, replacing a hardcoded600.Bug fixes
Service#datasets,Service#as_http_api,Dataset#structure,Dataset#describeall always raised (ArgumentErrororURI::InvalidComponentError) for anyone who called them — confirmedunused by every current consuming app, which is why none of this was
caught until now.
ServiceException#service_messagealways returnednildue to a typo(
@service_msgvs@service_message) — the exact accessor consumingapps call in their rescue blocks.
Service#ok?(unreachable, Faraday's ownraise_errormiddleware already handled every case it covered, and it would have
crashed itself if it ever ran) and a dead, non-functional
authparameter.
yajl-rubydependency: response bodies were beingre-serialized and re-parsed a second time for no reason, and the gem's
own
require "yajl"had been silently commented out, meaning it onlyever worked by accident in this repo's own test suite.
faraday-encodingdependency.Impact
Any app on 1.x will need a small migration when adopting 2.0.0. Primarily this
will involve updating
ActiveSupport::Subscriber#attach_tofrom:apito:data_services_apiif using notification-based subscribers (silentfailure otherwise, not a crash), and reviewing rescue clauses that may now
correctly catch
ServiceExceptionfor cases they previously missed.Testing
Full test suite green, rubocop clean. Manually verified end-to-end against
a running
ppd-explorercheckout: connection failures, service exceptions,and retries all instrument and log correctly with no crash.