perf: batch log/metric delivery instead of one blocking HTTP call per event - #4
Open
mleczakm wants to merge 1 commit into
Open
perf: batch log/metric delivery instead of one blocking HTTP call per event#4mleczakm wants to merge 1 commit into
mleczakm wants to merge 1 commit into
Conversation
… event Every logger()->info()/metrics()->set() call fired its own synchronous Guzzle request with a 5s timeout. Under high log/metric volume, or when the logdash API is slow/unreachable, this serialized directly onto the request path and could stall the host app for seconds per call - exactly the TODO the code already flagged (queue/retry/batching). - Add RequestQueue: buffers items and flushes on batch size, a lazy time interval, or process/request shutdown, with bounded retry. - HttpLogSync now posts buffered logs to POST /logs/batch (the endpoint the Node SDK already uses) instead of one POST /logs per line. - Metrics now dispatches queued updates concurrently via a Guzzle Pool (curl_multi) instead of sequentially; failures stay isolated per-item and are never retried, since `mutate` isn't idempotent. - Split connect_timeout (2s) from the overall timeout (5s) so a fully unreachable host fails fast instead of hanging the connect phase for the whole 5s budget. - httpClient is now injectable for testing. 200 send() calls now produce 8 HTTP requests instead of 200 (verified via a MockHandler-based benchmark).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Every
logger()->info()/metrics()->set()/metrics()->mutate()call fired its own fully synchronous Guzzle request with a 5s timeout, directly on the caller's code path - thesend()/sendMetric()methods carried a literal// TODO: queue, retry, batching. Under high log/metric volume, or when the logdash API is slow or unreachable, this could stall the host application for up to 5 seconds per call, serially.What
RequestQueue(new): buffers items in memory and flushes them when a batch-size threshold is hit, when a flush interval has elapsed, or when the PHP process/request shuts down (viaregister_shutdown_function). Includes bounded retry for transient failures.HttpLogSync: now buffers log lines and posts them toPOST /logs/batch(the same endpoint the Node SDK already uses) instead of issuing onePOST /logsper line.Metrics: now dispatches queued metric updates concurrently via aGuzzleHttp\Pool(curl_multi under the hood) instead of one sequential blockingPUTper update. Failures stay isolated per item and are not retried at the batch level, sincemutateis not idempotent and retrying a whole batch could double-apply an increment.connect_timeout(2s) from the overalltimeout(5s) on both clients, so a fully unreachable host fails fast on connect instead of burning the whole 5s budget just to establish a connection.httpClientis now an injectable constructor param on both classes (optional, defaults unchanged) so they're unit-testable withMockHandlerinstead of hitting the network.Public API (
LogSync/BaseMetricsinterfaces,send()/set()/mutate()signatures) is unchanged - this is transport-layer only. Both classes gained an optionalflush()method for callers (e.g. long-running workers) that want to force a send before going idle, instead of waiting on the batch size/interval/shutdown triggers.Proof
A
MockHandler-based benchmark: 200send()calls now produce 8 HTTP requests instead of 200, with all 200 log lines delivered intact (sequence numbers preserved). New tests cover queue batching/retry logic, the/logs/batchpayload shape, and that one failing metric in a batch doesn't block the others.Testing
composer test- 14/14 passing (was 5/5; addedRequestQueueTest,HttpLogSyncTest,MetricsTest)composer phpstan- level 8, no errorscomposer cs- PSR-12, clean./vendor/bin/parallel-lint src tests- no syntax errors