Add ExternalApiResponseBody tracker - #13
Conversation
external_api_response only tags metrics by host/status, which is not enough for APIs that route every call through a single endpoint (e.g. JSON-RPC style) and encode the actual method and error code in the request/response JSON body instead of the HTTP status. This tracker counts requests and extracts extra tags from those bodies via dot-notation paths, so calls can be split by things like the RPC method or an application-level error code. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Run the repo's ecs ruleset over the new tracker: expand inline associative arrays to multiline, break method chains onto their own lines, and rewrap an overlong doc comment. Also extract the repeated request construction in the feature test into a request() helper, matching the helper style of the existing ExternalApiResponse feature test. No behaviour change: `ecs check` is clean on these files and all 12 new tests still pass on PHP 7.4. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
nwdles
left a comment
There was a problem hiding this comment.
Approving. Reviewed the tracker against the existing ExternalApiResponse implementation and ran the suite locally on PHP 7.4 (PHPUnit 9.6.3), since CI could not do it — see the note at the bottom.
What I checked
- Follows the established tracker shape:
Installer+GuzzleClientOnStatsCallbackCreator+Closure::fromCallable, withresolveAdapter($connection, Counter::TYPE), so it works both on Prometheus and on the influx/telegrafEventAdapterpath. - The label set is stable, which is the part that matters most for a Prometheus counter:
extract()returns a value for every configured field even when the message isnullor the body is not valid JSON. Without that, the first network-level failure would register a different label set and blow up. Good call. - Body reading is safe: non-seekable streams are skipped,
max_body_bytescaps the read, andrewind()infinallyleaves the stream where the application expects it. - Backwards compatible:
request_fields/response_fieldsdefault to empty and the callback has to be wired into a client by hand, so existing consumers see no new metrics until they opt in. - New namespace means the
GuzzleClientOnStatsCallbackCreatorsingleton does not collide with the existing one. - 29 tests pass (17 before, 12 new). The one remaining failure is pre-existing on
master, unrelated to this PR — details below.
What I pushed
One commit, 1eac46c, applying the repo's own ecs ruleset to the four new files — the inline associative arrays (['code' => 'error.code'] and friends) violated umbrellio/code-style-php, so vendor/bin/ecs check was failing on all four. I wrote the formatting by hand rather than committing ecs --fix output, because the autofixer mangles nested arrays inside function calls (it leaves whitespace-only lines and collapses closers into ], ]));). I also pulled the thrice-repeated request construction in the feature test into a request() helper, mirroring the helper style of the existing ExternalApiResponse feature test. No behaviour change; ecs check is now clean on these files.
Non-blocking nits, for whenever you next touch this
max_body_bytesis only enforced whengetSize() !== null. A chunked response withoutContent-Lengthcan report an unknown size, and then the body is read in full regardless of the cap. Best-effort is fine, but the README currently reads like a hard guarantee.- The README section documents the Prometheus metric format but not the Influx one;
external_api_responsedocuments both, and this tracker supports both adapters. - "exactly like
external_api_responsedoes for itsmain_metric" is slightly off — that tracker's stream guard works off missing handler stats, not off stream seekability. DEFAULT_VALUE/DEFAULT_MAX_BODY_BYTESare duplicated between the class constants andconfig/event_tracker.php.
Note on CI
The red checks here are not this PR's fault. Lint targets ubuntu-20.04, which no longer exists as a runner, so the job never starts and is cancelled after ~24h; Test and Code coverage depend on it via needs, so they are skipped. build-and-push-image fails on pushes to master too. The practical consequence is that the test suite has not actually run in CI for about a year, which is how the dontWriteEveryRequestIfRedirect failure went unnoticed since 2.0.2 — filed separately, not something this PR needs to address.
Why
TS-5743 added a metric for tracking Atlant response codes, but the existing
external_api_responsetracker only tags metrics byhost/status. Atlant is a JSON-RPC style API — every call goes through a single endpoint, and the actual method name and business error code live in the request/response JSON body, not in the HTTP status. With the current tracker it's impossible to split requests by method.What
Adds a new tracker,
ExternalApiResponseBody, alongside the existingExternalApiResponse:on_stats, same integration pattern asExternalApiResponse.request_fields/response_fieldsconfig, powered bydata_get()), tagging a Counter metric with them alongsidehost/status.max_body_bytes, and rewinds the stream after reading so the rest of the application can still consume the response body normally.EventTrackerServiceProvider, with a default config entry and README section documenting integration (including how to combine it withExternalApiResponseon the same Guzzle client).Testing
MessageBodyFieldsExtractorTest(unit) andGuzzleClientOnStatsCallbackCreatorTest(feature).methodandcode.