Skip to content

feat: batch delivery off the request path and release automatically - #7

Merged
szymeo merged 3 commits into
mainfrom
malini/01J2242A9F1CC2C42A9A073AAA
Oct 24, 2026
Merged

szymeo merged 3 commits into
mainfrom
malini/01J2242A9F1CC2C42A9A073AAA

Conversation

@szymeo

@szymeo szymeo commented Oct 24, 2026 •

Copy link
Copy Markdown
Member

Summary

feat: batch delivery off the request path and release automatically

Publish malini/01J2242A9F1CC2C42A9A073AAA against main.

  • .github/workflows/ci.yml
  • .github/workflows/pr-title.yml
  • README.md
  • check-deployed-package/check.php
  • scripts/version.ts
  • src/Logdash.php
  • src/Queue/RequestQueue.php
  • src/Sync/HttpLogSync.php
  • tests/LogdashTest.php

Validation

  • No validation results were reported for this session.

@szymeo szymeo left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lead review: logdash moved from services to domains, so the smoke test must use the domain model instead of a project.

  • LOGDASH_API_KEY becomes the ingest key of one shared domain, "SDK smoke tests" (its Settings page, "Ingest key": one write-only key for every app on the domain). The owner adds it, replacing this repo's old per-service key.
  • Send with the SDK's app option set to php-sdk, so each SDK shows up as its own app on that domain.
  • LOGDASH_PERSONAL_API_KEY is a read-only key (Logs read, Metrics read) restricted to that domain. GET https://api.logdash.io/personal-api-keys/whoami returns access: { kind: "clusters", ids: [domainId] }. Fail with a clear message unless the kind is clusters and there is an id.
  • Read logs from GET /clusters/{domainId}/logs?apps=php-sdk (narrowed to this run's seed) and metrics from GET /clusters/{domainId}/metrics (each entry has app, name, value: match on app and name), both with the same bearer. Drop /projects/{id}/....
  • Never print an API key, not even one CI masks.

Check paths, query params and response shapes against logdash-io/logdash.io: apps/backend/src/log/core/log-core.controller.ts (readClusterLogs, ReadClusterLogsQuery in log/core/dto/cluster-logs.query.ts) and apps/backend/src/metric/core/metric-core.controller.ts (readClusterMetrics). The check program needs the app option, so it runs against the version this PR releases; before that, a manual run on this branch is not expected to pass.

@szymeo szymeo left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lead review: three blocking code findings and several minor ones. Payloads match the backend DTOs and the release computes 1.1.0, but each blocker below makes this release lose data that 1.0.6 delivered.

BLOCKING

  • src/Sync/HttpLogSync.php ~13-15, 30: cutting messages at 16 KB does not keep 100 logs under the API's 2 MB body limit (backend main.ts ~20), because JSON escapes quotes and slashes. Logging 100 arrays sent a 2,129,210-byte batch; the API answers 413, which is not retried, so all 100 logs are lost. In RequestQueue::log() add up strlen(json_encode($log, FLAGS)) and flush before about 1.5 MB, and cut messages to the 4096 characters the API keeps.
  • src/Queue/RequestQueue.php ~122-126: age is counted from the oldest buffered item, so a single log in a long-running worker waits for the next log or for exit and is lost on SIGKILL or OOM ("job 1 failed" had not reached the API after 3 s idle). Use #4's $lastFlushAt, set in the constructor and in flush(), and flush when hrtime(true) - $lastFlushAt >= 1 s.
  • src/Queue/RequestQueue.php ~39-50: the httpClient is no longer injectable, as it was in #4. Add ?ClientInterface $httpClient = null to RequestQueue and an httpClient option to Logdash::create, and send project-api-key and Content-Type per request instead of as client defaults.

MINOR

  • RequestQueue.php ~103-108, 168: timeouts are retried, so one log against an API that never answers blocks for 10.2 s (1.0.6: 5 s), and a retry can duplicate a batch the API already stored. Retry only on ServerException.
  • RequestQueue.php ~111-120: after pcntl_fork, the child's shutdown flush sends the parent's buffer again (a buffered mutate applies twice). Store getmypid() and drop the buffer when it changes.
  • check-deployed-package/check.php ~52-55: set and mutate merge into one set, so the smoke test never sends a change. Call flush() between them.
  • Release re-runnable: gh release view "v$V" >/dev/null 2>&1 || gh release create ..., then the Packagist ping, so a rerun after a failed step finishes.
  • pr-title.yml: permissions: {} above jobs:. scripts/version.ts ~4: the comment still names tony's Info.plist; say Packagist reads the tag.

Add a test for each blocking fix. LOGDASH_PERSONAL_API_KEY is coming from the owner.

@szymeo
szymeo marked this pull request as ready for review October 24, 2026 21:44
@szymeo
szymeo merged commit 84b58de into main Oct 24, 2026
7 checks passed
@szymeo
szymeo deleted the malini/01J2242A9F1CC2C42A9A073AAA branch October 24, 2026 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant