Release 2.0.0: stable exception contract, observable retries, dead-code and bug removal

CHANGELOG.md

@@ -7,6 +7,108 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## Unreleased
 
+## 2.0.0
+
+### Documentation
+
+- Documented the exact payload shape of every notification event: added a
+  plain-prose comment above each `instrument_*` method in `service.rb`
+  describing its payload. Writing this down surfaced several undocumented 
+  inconsistencies that are noted explicitly rather than fixed: 
+  `requests.data_services_api`'s payload is a raw `Faraday::Env`, not a
+  Hash like every other event; `duration` (milliseconds, `Integer`) and 
+  `will_retry_in` (seconds, `Float`) describe similar things in different
+  units; `path` is a bare `String` on most events but only derivable from
+  a `URI` object via `response:` on `response.data_services_api`;
+  `status` can be `nil` on `service_exception.data_services_api`;
+   `query_string` is always `nil` for POST-triggered failures
+
+### Changed
+
+- **Breaking**: `Faraday::ResourceNotFound`/`Faraday::ClientError`/
+  `Faraday::ServerError`/`Faraday::ParsingError` (any 4xx/5xx status, or an
+  unparseable response body) are no longer raised directly to callers.
+  They're now always wrapped in `DataServicesApi::ServiceException` before
+  being raised, restoring the exception contract that consuming
+  applications were already written against (`e.service_message`, `e.status`)
+  but that this gem had stopped actually providing
+- Fixed `ServiceException#service_message`, which always returned `nil` due
+  to a typo (`initialize` assigned `@service_msg` instead of `@service_message`)
+- `service_exception.data_services_api` now fires for this whole class of
+  failure (previously only `Faraday::ResourceNotFound`/404), and its
+  `query_string` field is now populated correctly from the actual request
+  params instead of always being `nil`
+- Added a `retry.data_services_api` notification, fired immediately before
+  each retry attempt on a network failure, with `path`, `method`,
+  `retry_count`, `exception`, and `will_retry_in`
+- Fixed `Dataset#structure` and `Dataset#describe`, which always raised
+  (`ArgumentError` or `NoMethodError` on `nil`) for any `Dataset` obtained the
+  normal way via `Service#dataset(name)`. That method only ever populated
+  `data-api`/`dataset` in the JSON it hands to `Dataset`, never
+  `structure-api`/`describe-api`, so `structure_api`/`describe_api` were
+  always `nil`. Both are now derived from `data-api` the same way the real
+  `/dataset` listing endpoint returns them (`<data-api>/structure`,
+  `<data-api>/describe`). `Dataset#structure` also called `api_get_json`
+  with a missing required argument, the same class of bug as `Service#datasets`
+- **Breaking**: `Service` no longer does any logging of its own. The `logger:`
+  config option has been removed, along with the automatic `Rails.logger`
+  wiring, the `puts` debug line, and all `logger.info`/`error`/etc calls.
+  Consuming applications should subscribe to the gem's
+  `ActiveSupport::Notifications` events instead and log whatever they need,
+  at whatever level and format they choose
+- **Breaking**: instrumentation event names are now namespaced under
+  `data_services_api` instead of the generic, collision-prone `.api` suffix:
+  `requests.api` -> `requests.data_services_api`,
+  `response.api` -> `response.data_services_api`,
+  `connection_failure.api` -> `connection_failure.data_services_api`,
+  `service_exception.api` -> `service_exception.data_services_api`. Fields
+  that used to only be visible in the removed log output (request path,
+  method, status, returned row count) are all derivable by subscribers from
+  the `Faraday::Response` object already included in `response.data_services_api`'s
+  payload, so they aren't duplicated as separate event fields
+- **Breaking**: Faraday's built-in request/response logging middleware is no
+  longer enabled automatically in Rails. It's now opt-in via
+  `Service.new(faraday_logger:)`, passing a logger object to hand to Faraday.
+  When enabled, it still defaults to logging at `debug` level with headers/
+  bodies/errors off (matching the old always-on behaviour), configurable via
+  `Service.new(faraday_logger_options:)`
+- Fixed a bug where `service_exception.api`/`connection_failure.api` were
+  never logged outside of a Rails environment; the new notification events
+  fire consistently regardless of environment
+- Fixed a `NameError` (`RACK::Exception` instead of `Rack::Exception`) in the
+  service-exception error path that would raise whenever a `Faraday::ResourceNotFound`
+  without a `status` reached it
+- Removed the `yajl-ruby` dependency and the `Service#parser`/`parse_json`
+  machinery built on it. Response bodies are already parsed to Ruby
+  Hash/Array by Faraday's own `:json` response middleware; the removed code
+  was re-serializing that result back to a JSON string and parsing it a
+  second time with Yajl for no benefit. This also fixes the gem being broken
+  out of the box for any consumer that didn't separately `require 'yajl'`
+  themselves, since this gem's own `require "yajl"` had been commented out
+- Removed the unused `faraday-encoding` dependency; nothing in the gem
+  configures Faraday's `:encoding` middleware
+- Fixed `Service#datasets`, which always raised `ArgumentError` (it called
+  `api_get_json` with a missing required argument). Confirmed unused by
+  every consuming app currently on this gem, which explains why it went
+  unnoticed
+- Fixed `Service#as_http_api`, which raised `URI::InvalidComponentError`
+  whenever `url:` was configured with a scheme (exactly as the README's own
+  usage example shows) and a relative path was passed to `api_get_json`/
+  `api_post_json`. Also unused by any current consumer, since all existing
+  calls happen to pass a full URL rather than a relative path
+- Removed `Service#ok?`, which was unreachable in practice (Faraday's
+  `raise_error` middleware already raises on all 4xx/5xx before `ok?` could
+  run) and would have raised a `TypeError` itself if it ever did run, since
+  `response.body` is already a parsed Hash by that point, not a JSON string
+- Removed the dead, non-functional `auth` parameter from the private
+  `create_http_connection`; no caller passed `auth: true`, and the
+  `api_user`/`api_pw` methods it referenced don't exist
+- Added a `connection_timeout` config option (defaulting to the previous
+  hardcoded `600` seconds) for consistency with the other configurable
+  retry/timeout options
+- Extracted the duplicated request-timing/instrumentation/rescue logic in
+  `get_from_api`/`post_to_api` into a shared `perform_request` helper
+
 ## 1.7.0 - 2026-07-13
 
 ### Added

Gemfile.lock

@@ -1,13 +1,11 @@
 PATH
   remote: .
   specs:
-    data_services_api (1.7.0)
+    data_services_api (2.0.0.prerelease)
       faraday (~> 2.13, >= 2.13.0)
-      faraday-encoding (~> 0.0, >= 0.0.6)
       faraday-follow_redirects (~> 0.4, >= 0.4.0)
       faraday-retry (~> 2.0, >= 2.0)
       json (~> 2.0)
-      yajl-ruby (~> 1.4)
 
 GEM
   remote: https://rubygems.org/
@@ -30,8 +28,6 @@ GEM
       faraday-net_http (>= 2.0, < 3.5)
       json
       logger
-    faraday-encoding (0.0.6)
-      faraday
     faraday-follow_redirects (0.5.0)
       faraday (>= 1, < 3)
     faraday-net_http (3.4.2)
@@ -95,7 +91,6 @@ GEM
       addressable (>= 2.8.0)
       crack (>= 0.3.2)
       hashdiff (>= 0.4.0, < 2.0.0)
-    yajl-ruby (1.4.3)
 
 PLATFORMS
   ruby

README.md

@@ -106,13 +106,22 @@ into the legacy DsAPI JSON format.
 all optional except `url`:
 
 - `url` - the base URL of the Sapi-NT API to query against
-- `instrumenter` - an object responding to `instrument(name, payload)`, used
-  to emit the `response.api`, `connection_failure.api` and
-  `service_exception.api` notifications described below. Defaults to
-  `ActiveSupport::Notifications` when running under Rails, otherwise `nil`
-- `logger` - an object responding to the standard `Logger` levels
-  (`info`, `warn`, `error`, `debug`), used to log request/response details.
-  Defaults to `Rails.logger` when running under Rails, otherwise `nil`
+- `instrumenter` - an object responding to `instrument(name, payload, &block)`
+  (e.g. `ActiveSupport::Notifications`), used to emit the notifications
+  described below. Defaults to `ActiveSupport::Notifications` when running
+  under Rails, otherwise `nil`. The gem itself never logs anything: consuming
+  applications are expected to subscribe to these notifications and log
+  whatever they need, in whatever format and at whatever level they choose
+- `faraday_logger` - an object responding to the standard `Logger` levels
+  (`info`, `warn`, `error`, `debug`). When given, enables Faraday's own
+  request/response logging middleware, passing this object to it. Not
+  enabled unless explicitly configured
+- `faraday_logger_options` - options passed to Faraday's logging middleware
+  (`headers`, `bodies`, `errors`, `log_level`), only relevant when
+  `faraday_logger` is also given. Merged over the default of
+  `headers: false, bodies: false, errors: false, log_level: :debug`, so
+  Faraday's request/response one-liners log at `debug` by default, staying
+  quiet unless the consuming app turns its own logger's level down
 - `connection_failed_retry_options` - a hash of
   [`faraday-retry`](https://github.com/lostisland/faraday-retry) options
   (`max`, `interval`, `interval_randomness`, `backoff_factor`) applied to
@@ -128,6 +137,26 @@ all optional except `url`:
 `Faraday::ResourceNotFound` (404) responses are not retried, since a 404 is
 not a transient failure.
 
+### Errors
+
+`Service` raises two kinds of exception, depending on where the failure
+happened:
+
+- **Network-level failures** — the request never got a response at all —
+  raise Faraday's own `Faraday::TimeoutError` / `Faraday::ConnectionFailed`
+  directly (after retries are exhausted). These are left as-is rather than
+  wrapped, since they're about the transport, not the API
+- **Application-level failures** — the remote API responded, but with an
+  error status (any 4xx/5xx) or a body that couldn't be parsed as JSON —
+  are always raised as `DataServicesApi::ServiceException`, never as the
+  underlying Faraday exception (`Faraday::ResourceNotFound`,
+  `Faraday::ClientError`, `Faraday::ServerError`, `Faraday::ParsingError`,
+  etc). `ServiceException` exposes `status` (the HTTP status code, where
+  available), `service_message` (the underlying error detail), and `source`
+  (the URL that was requested), giving consuming applications one stable
+  type to rescue for this whole category, regardless of which Faraday
+  version or exact status code is behind it
+
 ---
 
 ## Developer notes
@@ -205,10 +234,153 @@ local credentials required.
 
 ### Prometheus monitoring
 
-This gem integrates with Prometheus monitoring by emitting the following
-`ActiveSupport::Notification`s:
+This gem integrates with Prometheus monitoring, and supports general-purpose
+logging, by emitting the following `ActiveSupport::Notification`s via the
+configured `instrumenter`:
+
+- **`requests.data_services_api`** - emitted by Faraday's own instrumentation
+  middleware, for every request. **Its payload is not a Hash** like every
+  other event below - it's the raw `Faraday::Env` for the request (read
+  fields via its own accessors, e.g. `env.method`, `env.url`), so a
+  subscriber to this event needs different handling than the rest.
+- **`request.data_services_api`** - fires immediately before a request is
+  sent (both GET and POST), regardless of how it later resolves.
+- **`response.data_services_api`** - fires for every response that doesn't
+  raise (both GET and POST).
+- **`connection_failure.data_services_api`** - a network-level failure
+  (timeout or refused connection), after retries are exhausted. The request
+  never got a response at all.
+- **`service_exception.data_services_api`** - the remote API responded, but
+  with an error status (any 4xx/5xx) or an unparseable body. The exception in
+  this payload is always a `DataServicesApi::ServiceException` - Faraday's own
+  exception types are wrapped before a subscriber ever sees them.
+- **`retry.data_services_api`** - fired immediately before each retry attempt
+  on a network failure (not for `service_exception`-class failures, which
+  aren't retried).
+
+Payload fields, by event (fields are Hash keys except where noted; `-` means
+the event doesn't include that field):
+
+| Field | Type | requests<sup>†</sup> | request | response | connection_failure | service_exception | retry |
+|---|---|---|---|---|---|---|---|
+| `response` | `Faraday::Response` | - | - | ✓ | - | - | - |
+| `exception` | see note | - | - | - | `Faraday::TimeoutError`/`ConnectionFailed` | `ServiceException` | see note |
+| `path` | `String` (bare path, no scheme/host/query) | - | ✓ | -<sup>‡</sup> | ✓ | ✓ | ✓ |
+| `query_string` | `String`, nilable<sup>§</sup> | - | ✓ | -<sup>‡</sup> | ✓ | ✓ | - |
+| `method` | `String`, upcased | - | ✓ | -<sup>‡</sup> | - | - | ✓ |
+| `status` | `Integer`, nilable | - | - | -<sup>‡</sup> | always `503` | nilable<sup>¶</sup> | - |
+| `duration` | `Integer`, **milliseconds** | - | - | ✓ | ✓ | ✓ | - |
+| `will_retry_in` | `Float`, **seconds** | - | - | - | - | - | ✓ |
+| `retry_count` | `Integer`, 1-indexed | - | - | - | - | - | ✓ |
+| `returned_rows` | `Integer`, nilable | - | - | -<sup>‡</sup> | - | - | - |
+
+<sup>†</sup> `requests.data_services_api`'s payload is a `Faraday::Env`, not a
+Hash - none of these field names apply; see above.<br>
+<sup>‡</sup> derivable from the `response:`/`exception:` object already in
+the payload rather than duplicated as a separate field - see the code
+examples below.<br>
+<sup>§</sup> `nil` for POST requests (which never have query params) and for
+GET requests with no params.<br>
+<sup>¶</sup> `nil` if Faraday never associated a response with the error
+(`Faraday::Error#response_status` returns `nil` in that case - can happen
+for some `Faraday::ParsingError`s).
+
+**`duration` (milliseconds) and `will_retry_in` (seconds) use different units
+and types** - both describe elapsed/remaining time, but come from different
+underlying sources (this gem's own timing vs. `faraday-retry`'s own values
+passed straight through) and were never normalized against each other. This
+is an inconsistency, not an intentional design choice - don't assume the two
+are interchangeable.
+
+**`exception` in `retry.data_services_api`** is normally a raised exception
+(`Faraday::TimeoutError`/`ConnectionFailed`), but per `faraday-retry`'s own
+design it would be the synthetic `Faraday::RetriableResponse` if a
+status-code-based `retry_statuses:` option were ever configured. This gem
+doesn't set that option today, so in practice it's always a real exception -
+but that's this gem's current configuration, not a structural guarantee.
+
+### Subscribing to hooks in a Rails app
+
+The simplest way to subscribe is a block, registered once in an initializer
+(e.g. `config/initializers/data_services_api.rb`):
+
+```ruby
+ActiveSupport::Notifications.subscribe('response.data_services_api') do |*, payload|
+  Rails.logger.info(duration: payload[:duration], status: payload[:response].status)
+end
+
+ActiveSupport::Notifications.subscribe('service_exception.data_services_api') do |*, payload|
+  Rails.logger.error(
+    message: "API service exception: #{payload[:exception].message}",
+    path: payload[:path],
+    status: payload[:status]
+  )
+end
+```
+
+For anything beyond a line or two, an `ActiveSupport::Subscriber` is the more
+idiomatic Rails pattern — one method per event, matched by name
+(`attach_to :data_services_api` routes `response.data_services_api` to a
+`#response` method, and so on):
+
+```ruby
+# app/subscribers/data_services_api_subscriber.rb
+class DataServicesApiSubscriber < ActiveSupport::Subscriber
+  attach_to :data_services_api
+
+  def response(event)
+    response = event.payload[:response]
+    Prometheus::Client.registry.get(:api_status)
+                      .increment(labels: { status: response.status.to_s })
+    Prometheus::Client.registry.get(:api_response_times)
+                      .observe(event.payload[:duration])
+  end
 
-- `response.api` - API response, including status code and duration
-- `connection_failure.api` - failure to connect to the API, with exception
-  detail
-- `service_exception.api` - failure to process the API response
+  def connection_failure(event)
+    exception = event.payload[:exception]
+    Prometheus::Client.registry.get(:api_connection_failure).increment
+    Rails.logger.error(message: "API connection failure: #{exception.message}", status: 503)
+  end
+
+  def service_exception(event)
+    exception = event.payload[:exception]
+    Prometheus::Client.registry.get(:api_service_exception).increment
+    Rails.logger.error(message: "API service exception: #{exception.message}", status: event.payload[:status])
+  end
+
+  def retry(event)
+    Rails.logger.warn(
+      message: "Retrying #{event.payload[:method]} #{event.payload[:path]} " \
+                "(attempt #{event.payload[:retry_count]}) in #{event.payload[:will_retry_in]}s",
+      exception: event.payload[:exception].class.name
+    )
+  end
+end
+```
+
+`attach_to :data_services_api` in the class body (as above) subscribes
+immediately when the class loads — no separate initializer call needed.
+A subscriber under `app/subscribers/` is autoloaded the first time it's
+referenced; since nothing in the app calls
+`DataServicesApiSubscriber` directly, eager loading it in
+production (Rails does this automatically for `app/` in production) is
+what makes sure it's actually loaded, and therefore attached, before any
+requests are served. In development, where eager loading is off,
+reference the class once from an initializer instead, so it's guaranteed
+to load (and attach) at boot rather than on first use:
+
+```ruby
+# config/initializers/data_services_api.rb
+DataServicesApiSubscriber
+```
+
+**Deriving the returned row count** (see the note on `response.data_services_api`
+above — it isn't a separate field):
+
+```ruby
+def response(event)
+  response = event.payload[:response]
+  returned_rows = response.body['items']&.size
+  # ...
+end
+```

data_services_api.gemspec

@@ -27,9 +27,7 @@ Gem::Specification.new do |spec|
   }
 
   spec.add_dependency 'faraday', '~> 2.13', '>= 2.13.0'
-  spec.add_dependency 'faraday-encoding', '~> 0.0', '>= 0.0.6'
   spec.add_dependency 'faraday-follow_redirects', '~> 0.4', '>= 0.4.0'
   spec.add_dependency 'faraday-retry', '~> 2.0', '>= 2.0'
   spec.add_dependency 'json', '~> 2.0'
-  spec.add_dependency 'yajl-ruby', '~> 1.4'
 end