Repository navigation
Improve Sentry instrumentation, coroutine isolation and performance - #662
Merged
Merged
Conversation
…ravel Start the Sentry reconciliation against getsentry/sentry-laravel master at 0537a1d5bfd583beb89001887af9cadd3116bccb (4.29.0 plus one merge). The package now requires sentry/sentry ^4.34, the SDK release that sentry-laravel builds on. Its manifest also declares hypervel/coordinator and hypervel/core, which the event handler uses directly. Five feature classes take their upstream names: LogIntegration, CacheIntegration, QueueIntegration, NotificationsIntegration and ConsoleSchedulingIntegration. The shipped features list follows upstream's order and explains that applications can remove a feature or add their own. The provider regains upstream's explanatory comments, and the SDK identifier comes from Version::SDK_IDENTIFIER directly instead of a getter that only returned the constant. getSdkVersion() stays because split installs can run a package version that differs from the framework version. The test base is now TestCase, matching upstream's name and structure. ClientBuilderDecoratorTest is ported, and the provider tests that read the client depend on testIsBound as upstream's do. Validation: the Sentry suite under ParaTest, composer lint:fix and composer analyse.
… sentry-laravel Reconciles the shipped config, the data collection option, the config option tests and the console commands with getsentry/sentry-laravel master at 0537a1d5bfd583beb89001887af9cadd3116bccb. Config: - Add the `data_collection` option (default null), passed through to the SDK. - Default the additional SDK `integrations` to an empty list, as upstream does. The previous Transaction, FrameContextifier and Environment entries were installed even with `default_integrations` disabled. - Remove the shipped ValidationException `ignore_exceptions` default. The framework exception handler already never reports it; explicit captures now reach Sentry unless the application lists it. - Restore upstream comment wording, adapted to Hypervel's request lifecycle. Commands: - The about command shows the data collection status and marks `send_default_pii` as ignored once `data_collection` is set. - `sentry:publish` reads `.env` entries with the dotenv parser instead of a line regex. Exported, spaced and multiline assignments are now recognized, so declining PII or performance monitoring overrides an existing value and an existing DSN is not requested again. A key counts as set only when its last assignment has a value: upstream treated an empty `SENTRY_HYPERVEL_DSN=` as set, skipped the DSN prompt and could finish with Sentry disabled. - Type the test command's SDK logger and register the commands in upstream order. Tests: - Port the data collection config option test and move the container and integrations config option tests to upstream's placement, adding upstream's OTLP integration case. - Adapt upstream's publish command regex tests to dotenv-valid dotted names, and add command-level regressions for exported, spaced, empty and multiline assignments. - Cover default integrations, callable integrations, the shipped breadcrumb and tracing fallbacks, and ValidationException capture with and without `ignore_exceptions`. The package now declares vlucas/phpdotenv, which the publish command uses directly, and no longer requires hypervel/validation. Verified with the Sentry suite under ParaTest, composer lint:fix and composer analyse.
…aravel Reconciles error events, request data, logging and the integration helpers with getsentry/sentry-laravel master at 0537a1d5bfd583beb89001887af9cadd3116bccb. Data collection: - The auth event listeners and SetRequestIpMiddleware follow the data collection policy's user info setting instead of `send_default_pii`. - SQL query breadcrumbs record query data through the SDK's database data collector. `sql_bindings` applies only when `data_collection` is not set. - The request fetcher ports upstream's parsed body handling for both modes. Session, remember-me and XSRF cookies are filtered through upstream's SensitiveCookieFilter. - The filter accepts integer cookie names. PHP stores numeric cookie names as integer array keys, which made a string-typed filter throw and fail event capture for any request carrying such a cookie. Integrations: - Rename ContextIntegration to HypervelContextIntegration, matching upstream. - Narrow the model violation origin to `?array` and type its callbacks. - LogsHandler imports Sentry's logger function. - Restore upstream docblocks and add missing method titles. Not ported: - Octane event handlers. Each request runs in its own coroutine with its own scope and transaction name, and the request context flushes its events. - SetRequestMiddleware. The fetcher converts the request only when an event needs it, instead of storing a PSR-7 copy in the worker-shared container. - withRawQueryString(). The PSR-7 bridge already builds the URI from the received query string. - sentryW3CTracingMeta(), deprecated upstream and always empty. - Lumen routing tests. Tests: - Port upstream's request data collection, SetRequestIpMiddleware and request fetcher cases, adapted to real requests in the request context. - Cover the auth listener, SQL query data and request IP middleware gating with data collection defaults and with user info disabled. - Add a regression test for numeric cookie names. Verified with the Sentry suite under ParaTest, composer lint:fix and composer analyse.
…-laravel Reconcile tracing, request and response span data, routing, views, database spans and the traces sample rate job middleware with getsentry/sentry-laravel master at 0537a1d5bfd583beb89001887af9cadd3116bccb. Ported: - With data collection configured, transactions record request headers, cookies and bodies, and response headers, cookies (filtered through SensitiveCookieFilter) and bodies. - Query spans record parameters through DatabaseDataCollector; the tracing sql_bindings option applies only without data collection. - Upstream tests for request and response data collection, the OTLP traces endpoint, MySQL URL connections, a missing tracing config key, database data collection, and trace propagation while tracing is disabled. Defect corrections: - Remove the app.bootstrap span. Workers boot once before serving requests, so on long-lived workers the span had a negative duration and middleware.handle started before its transaction. - Let coroutine cancellation propagate through the tracing event handler. Query origin resolution reads files through hooked I/O, which can be cancelled; the handler previously swallowed the cancellation. - The request fetcher no longer uses Symfony's PSR-7 bridge. The bridge decoded every JSON body on every sampled transaction and error event (about 7.5 ms per MB), but Sentry only reads that body for legacy error events within the size limit. The fetcher now builds the PSR-7 request directly and decodes only bodies the legacy collector can read. - Uploads the application moved no longer drop events. The bridge opened each upload by its real path and threw once it was moved, and the SDK discards events whose processors throw. Uploads are now converted by path without being opened. Adaptations: - Testbench requests run in their own coroutine, so the propagation test reads the scope inside the route. - Duplicate Hypervel tests are consolidated into the upstream tests, keeping their stronger assertions. - The view engine test uses ParallelTesting::tempDir(). - The view engine registration drops a compatibility catch for Laravel versions before 5.4. Validation: Sentry suite under ParaTest (467 tests), AfterEachTestSubscriber tests, composer lint:fix and composer analyse.
…ravel Bring the cache, Redis, queue and notification features in line with getsentry/sentry-laravel master (0537a1d5bfd583beb89001887af9cadd3116bccb) and fix span and scope handling for nested notifications and jobs. Cache and Redis: - Redis command tracing moves into CacheIntegration, as upstream has it, and RedisFeature is removed. Redis tracing, cache tracing and cache breadcrumbs keep independent switches; Redis events are enabled at boot only when Redis tracing is on. - Failed-command spans, null timings, pool data and the coroutine ID are kept. The duplicate db.statement and duration fields are dropped, which makes 100 Redis spans about 5 KB smaller and quicker to serialize. - Redis parameters follow the data collection policy (send_default_pii in legacy mode). - Session keys are read from the request cookie before constructing the session store. Upstream's cache-backed session handling is not ported: Hypervel has no cache-backed session handlers. Queue and notifications: - Queued job arguments are collected on publish and process spans when data collection allows it. - The notification breadcrumb listener is only registered when notification breadcrumbs are enabled. Defects fixed: - A skipped notification, or one that failed before sending, finished the span of the notification that sent it. Spans are now finished only by the notification attempt that started them, which also ignores the extra terminal event from channels that report a failure without throwing. - A job run synchronously inside another job (sync chains, dispatchSync) popped the parent job's scope, lost its breadcrumbs and attached later spans to the wrong parent. Failure events could also finish the parent's span or transaction. Jobs are now tracked per coroutine; a failed job's scope stays available for exception reporting until its attempt has ended and the next job starts or its parent completes. Tests: - Port QueueDataCollectionTest. Publish spans come from the JobQueueing and JobQueued events, which the sync queue does not raise, so the upstream cases raise them through a test queue; a real sync push confirms the process span without a publish span. - Merge the upstream cache and Redis tests with Hypervel's Redis tests. - Notification tests use the real config keys; upstream sets keys that do not exist. Verified with the Sentry suite under ParaTest, composer lint:fix and composer analyse.
Bring the console, scheduling and storage features in line with getsentry/sentry-laravel master (0537a1d5bfd583beb89001887af9cadd3116bccb) and fix how scheduled task check-ins are stored and finished. Console: - Once the data_collection option is set, command breadcrumbs record the arguments and options that differ from their definition defaults, as separate input.arguments and input.options arrays. Values pass through the SDK's denylist filter, so names such as password, token or secret are sent as [Filtered]. Upstream sends the raw command line here, which would leak credentials under the documented migration settings. - Without data_collection, raw input is still sent only when send_default_pii is enabled. Scheduling: - Upstream's useCacheStore() and its cache and Context hand-off of check-in IDs are not ported. Background tasks run in a coroutine of the scheduler process, which both starts and finishes their check-ins. Defects fixed: - Check-ins were kept in hidden application log context, which is copied into queued job payloads and scheduled subprocess environments. They now live in coroutine context and are removed when the task finishes. - The finishing check-in changed the status of the in-progress check-in object. With asynchronous delivery, the start check-in could be sent as ok or error. The finish now sends a copy. Tests: - Port ConsoleExceptionScopePropagationTest. - Merge the upstream scheduling tests: schedule override, the inspire command, background tasks run through schedule:run, and overlapping executions run concurrently. - Rename tests to their upstream names where Hypervel duplicated them. - Use dotted config keys in the console tests. Docs describe command input and Redis parameters under data_collection. Verified with the Sentry suite under ParaTest, composer lint:fix and composer analyse.
…econciliation # Conflicts: # tests/Sentry/Aspects/GuzzleHttpClientAspectTest.php
A custom driver that wraps another disk, such as Sentry's storage decorator around an S3 or Google Cloud Storage disk, received its config with the pool option stripped. The wrapped cloud disk then lost its pool.fingerprint, pool.name and pool options, so it fell back to automatic fingerprinting and default pool limits. Custom drivers the manager doesn't pool now receive the full disk config, including pool, so they can pass it on to Storage::build(). Custom drivers the manager pools, and the built-in drivers, still receive the config without pool, since the manager owns that pool. The custom filesystems documentation describes the difference. Verified with FilesystemManagerTest (the new regression fails without the fix) and the Filesystem suite under ParaTest.
AOP proxies are generated under the bootstrap cache path, so stack frames from a proxied vendor class, such as Guzzle's Client, report a file inside the application. Code that sorts frames into application and vendor code treated those frames as application code: the error page marked them as application frames, and Telescope could report a proxied vendor class as the caller of a query or request. GenerateProxies now records the canonical source file of each ordinary and replacement proxy once loaded-target validation succeeds, and exposes the map through proxySources(). The entries are kept across bootstraps, since already-loaded proxy classes stay in use, and are reset by flushState(). The map is bounded by the number of proxied classes. The error page's Frame::isFromVendor() and Telescope's stack trace caller lookup classify a proxy frame by its source file, while still showing the proxy frame's real file and line. Foundation now declares its existing direct use of hypervel/di. Verified with GenerateProxiesTest, FrameTest, PackageMetadataTest and the new QueryWatcherTest case (each fails without the change), and the Di, Foundation and Telescope suites under ParaTest.
Telescope's ClientRequestWatcher intercepts Guzzle's Client::transfer so it also sees raw Guzzle clients and SDKs, not only the HTTP client's events. It recorded an entry from on_stats, which Guzzle calls once per redirect hop, so a followed redirect produced an entry for every hop. A response Guzzle rejected (http_errors) was recorded without its response, and a blanket catch hid recording failures. The watcher now matches laravel/telescope 5.x (bedfc50a, checkpoint not advanced), which records one entry per logical request: - on_stats only keeps the last transfer stats and still calls the caller's own on_stats callback on every hop. - One entry is recorded when the transfer settles, from the last prepared request (or the entry request without stats) and the final response: the fulfilled response or the response carried by the rejection. Rejections are passed on unchanged. - duration is only set when Guzzle reports a transfer time. - Recording errors surface, like upstream and the other watchers. - Masking, size limits, hypervel_data, telescope_tags, ignore_hosts and telescope_enabled are unchanged. Guzzle 7 and 8 carry a rejected response on different exceptions. PendingRequest's existing extraction moves to Hypervel\Http\Client\Guzzle\RejectedResponse::from() so the watcher and Sentry's Guzzle aspect share it; responseFromException() delegates to it. Tests: - Restore upstream's HTTP client tests to their Http::fake() form. - Add regressions for a followed redirect recorded once with the final response, an error response rejected by Guzzle, and recording failures surfacing from the transfer. - Remove five Hypervel tests duplicated by the retained raw request, masking, failure and redirect coverage. Verified with ClientRequestWatcherTest (the redirect regression fails on the old watcher) and the Telescope and Http suites under ParaTest on Guzzle 8.2. On Guzzle 7.15.5 the Telescope suite passes; the Http suite segfaults in HttpClientDestinationPolicyTest from a Swoole 6.2.2 curl hook use-after-free that also occurs without these changes.
Bring HTTP client spans and breadcrumbs in line with getsentry/sentry-laravel master (0537a1d5bfd583beb89001887af9cadd3116bccb) and fix how the Guzzle aspect records redirected and rejected transfers. Instrumentation: - Upstream's HttpClientIntegration feature is not ported. The aspect on Guzzle's Client::transfer instruments every Guzzle client, including the HTTP client and SDKs built on Guzzle, and its spans keep the auto.http.guzzle origin. The no_sentry_aspect option still opts a client or request out, and the README records the difference. - The span and breadcrumb are recorded once, when the transfer settles, from the last prepared request and the final response. Previously on_stats recorded a breadcrumb for every redirect hop and finished the span with the first hop's response, and span data came from the request before Guzzle applied options such as query. The caller's on_stats still runs on every hop, rejections are passed on unchanged, and the Hub span is never replaced. Error responses rejected by Guzzle keep their status through the shared RejectedResponse helper. - When both span tracing and breadcrumbs are off, the aspect still adds trace headers but leaves the caller's on_stats unwrapped. Data collection: - Legacy mode keeps its fields and adds http.request.body.size. - With data_collection, spans and breadcrumbs add url.full and a filtered http.query, and spans add the SDK's request and response header, cookie and body collection. Origin: - Port upstream's http_client_requests_origin and http_client_requests_origin_threshold_ms options. Their config casts replace upstream's two consumer-coercion tests (REMOVED comments). - A synchronous request over the threshold resolves its origin from 40 frames when it settles, since its caller sits below the promise wait and the proxied Promise::wait() frames. An asynchronous request with a sampled span captures 64 frames when the transfer begins, because it can settle after its caller returns. The AWS SDK sends synchronous calls this way, below its middleware and the storage decorators. An SDK that queues the transfer itself reports the site that waits for it. - resolveEventOrigin() accepts a backtrace captured earlier; other features keep their 20-frame capture. - Generated AOP proxies are classified in or out of the application like their source files: after the client options are built, proxy paths are added to the SDK's include or exclude lists where the effective rules would classify a proxy differently from its source. An explicit include covering a proxy still wins. Package metadata declares the direct guzzlehttp/promises dependency. The CI Guzzle 7 job also runs the new HTTP client tests and Telescope's ClientRequestWatcherTest. The Sentry docs describe HTTP client coverage, the origin settings and no_sentry_aspect. Tests: - Port HttpClientIntegrationTest, HttpClientDataCollectionTest and the HttpClientOriginFixture, sending faked HTTP client requests in place of upstream's dispatched events. - Add redirect, query option, rejected error response, async coroutine ownership, SDK origin and disabled-output regressions to GuzzleHttpClientAspectTest, and a combined Sentry and Telescope origin test. - Add proxy classification cases to ServiceProviderTest, origin config casts to ConfigTest, and a decorated cloud disk pool regression to StorageIntegrationTest. Verified with composer lint:fix, composer analyse, and under ParaTest on Guzzle 8.2 the Sentry (520), Telescope, Filesystem, Http, Di, Foundation, Saloon and ApiClient suites. The redirect, origin depth, SDK origin and disabled-output regressions fail against the previous code. On Guzzle 7.15.5 the CI compatibility set (330 tests, 7 existing version-gated skips) and the Sentry, Telescope and Filesystem suites pass, and caller selection was checked against real cURL transfers on both families. The Guzzle 7 Http suite segfaults in HttpClientDestinationPolicyTest from a Swoole 6.2.2 curl hook use-after-free that also occurs without these changes, so Guzzle 7 validation is not complete. Timing comparisons belong to the later performance assessment.
Reconcile Sentry's Hub, child coroutine propagation, cancellation and
flushing with sentry/sentry 4.34.0 and getsentry/sentry-laravel master
(0537a1d5bfd583beb89001887af9cadd3116bccb).
Request inheritance:
- Child propagation cloned the parent's request into the child's
application request context. Created children gained a request the
framework does not give them, and forks lost the parent's request
identity. Children now get a reference under
HypervelRequestFetcher::REQUEST_CONTEXT_KEY, which only Sentry reads,
through HypervelRequestFetcher::currentRequest() (the coroutine's own
request first). Created grandchildren keep it; detached and delivery
coroutines get none. The request fetcher and the cache session key use
it.
- Without a request, the user IP set on Authenticated and Sanctum
TokenAuthenticated events came from the container's default request, a
fake 127.0.0.1. It now comes from currentRequest(), so no IP is set
without a request. Created children previously read the cloned real
request; they now read the inherited one, which the old make('request')
reader would miss after the propagation change. Upstream records the
console request's 127.0.0.1.
Hub:
- Match SDK 4.34.0: before_breadcrumb, traces_sampler and
profiles_sampler failures are logged and fall back as the SDK does, and
addAttachment() is added.
- Coroutine cancellation from those callbacks propagates unchanged.
- The class docblock requires comparing it with Sentry\State\Hub on every
SDK update. Methods have titles, and withScope() and captureCheckIn()'s
duration are natively mixed, since the SDK interface leaves them
untyped.
drainEvents() is marked internal and the per-class span context prefix
is protected. Flushing is unchanged: runtime contexts started for HTTP
requests, jobs, scheduled tasks and WebSocket events flush logs, metrics
and the transport when they end, console commands flush on terminating,
and worker stop and exit drain with a bounded timeout, so upstream's
extra flush points are not needed.
Tests:
- Add Hub callback failure, cancellation and attachment tests, auth IP
tests without a request and in a child coroutine, and propagation
assertions for the application context, captured request data, fork
identity and descendants.
- Two child-coroutine tests passed when the child threw, because a
timed-out channel pop returned false; they now assert the popped
result.
- Remove eight HttpPoolTransportTest tests that repeated the file's
remaining acquisition, release, failure and close coverage, and
CoroutineSafetyTest's fake context trait test. Replace the
sentryConfigWith() helper with dotted config keys.
Verified with composer lint:fix, composer lint, composer analyse and the
Sentry suite under ParaTest (519 tests). Against the previous code, the
Hub failure-fallback and attachment tests, the no-request auth IP test
and the changed propagation tests fail. The cancellation cases pass
there, since the old Hub called the callbacks directly; they protect the
new catch paths. The child-coroutine auth IP test also passes there,
since the old propagation cloned the request; it fails with the new
propagation and the old reader.
php-cs-fixer's no_unused_imports rule keeps an import when its name appears in any comment, ignoring case, so these imports survived formatting although no code or docblock uses them. Verified with composer lint and composer analyse, and the affected test files pass.
Compare every getsentry/sentry-laravel source file and the config at
master (0537a1d5bfd583beb89001887af9cadd3116bccb) with Hypervel's Sentry
package, including files reconciled earlier.
Source:
- Add return types to the untyped closures in the provider bindings,
routing dispatchers, FilesystemDecorator, LogIntegration's log
creators and Integration::handles().
- Add the missing method titles to the model violation reporters,
LogIntegration, WorksWithSpans and both handleBatch() methods.
- Remove the catch (BindingResolutionException) blocks around
make('events') in the provider. The base service providers always
register the event dispatcher, so they could never run.
- Restore upstream's method order in Integration and
Tracing\Middleware, and its comments in Tracing\Middleware and
BacktraceHelper. Correct the comment that blamed a missing compiled
view path comment on a very old Laravel version.
- Note where upstream's TransactionFinisher and Lumen route name
extraction are not ported: each request coroutine finishes its own
transaction, and Hypervel has no Lumen routing.
Kept: parameterless Feature::onBoot() hooks that resolve from the
container, modelHasAttribute() (Hypervel's hasAttribute() also counts
casts), and no normalizeKeyOrKeys(), since the cache repository
dispatches resolved key lists. The README differences remain accurate.
Set the sentry-laravel sync checkpoint to the compared revision, with
notes on the provider and FEATURES mapping, the feature hook shape,
comparing Hub with the SDK's on every SDK update, the packages Hypervel
does not have, and AI agent monitoring, which is not ported yet and not
covered by the checkpoint.
Verified with the Sentry test suite under ParaTest, composer lint:fix
and composer analyse.
SentryTracesSampleRate resampled Hub::getTransaction(), the root of whatever span was current. Job middleware runs after JobProcessing, so a job dispatched synchronously during a request, or by another job, changed the sampling of that enclosing transaction: with a rate of 0, a whole request trace or parent job transaction was dropped. With queue job spans disabled, a nested job did the same to its parent job's transaction. getsentry/sentry-laravel master (0537a1d5bfd583beb89001887af9cadd3116bccb) has the same code. QueueIntegration's job frames now hold the span they started instead of an ownsSpan flag, and the internal currentJobTransaction() returns the running job's span only when it is a transaction. The middleware resamples only that transaction, still never upsamples, and always calls the next handler. Its hub-bound check is gone: the lookup does not use the hub, and an inactive integration has no job frames. Upstream's five sampling tests resampled a transaction no job started. They keep their assertions and now start the transaction through JobProcessing and finish it with JobProcessed. Dispatch tests cover a root job's own transaction, a job inside another transaction and a nested job without a span; the latter two fail on the previous code. Verified with the middleware and queue integration tests, the Sentry suite under ParaTest, composer lint:fix and composer analyse.
Compare the Laravel section of getsentry/sentry-docs at master (4307693c8537fecfe123e80d50837c5c94979585) with the Hypervel Sentry page once, keeping Hypervel integration guidance and linking Sentry's documentation for general SDK usage instead of repeating it. Added: - Model violation reporters, linking browser traces with sentryMeta(), log batching with SENTRY_LOG_FLUSH_THRESHOLD, instrumentation toggles and spans in child coroutines. - Scheduled task monitor options, and why tasks filtered with between, unlessBetween, when or skip report missed check-ins. - That the job sampling middleware needs job transactions and leaves an enclosing transaction's sampling alone. - Sending OpenTelemetry traces to Sentry over OTLP, and linking Sentry events to them with OTLPIntegration. OtlpIntegrationTest checks that captured events carry the active OpenTelemetry trace and span IDs. - The data_collection option, custom features and client builder customization. Corrected: - The traces_sampler example used a closure, which blocks config:cache, and checked a route name that never matched, since HTTP transactions are named after the path when sampled. - Validation exceptions are dropped by the exception handler, not by the Sentry config. - Explicitly copied requests are shared with detached children, not cloned. - Request headers are sent without send_default_pii, with sensitive header values filtered, and other event data may still contain personal information. The porting guide gains a Sentry entry for the namespace, the SENTRY_LARAVEL_DSN rename (without it Sentry stays disabled) and SENTRY_LOG_LEVEL. The sentry-docs sync entry is removed: future documentation changes accompany package changes. Verified with OtlpIntegrationTest, the Sentry suite under ParaTest and composer lint:fix.
Excimer samples whichever coroutine's PHP stack is running, so a transaction profile includes work from other coroutines in the same worker, such as concurrent requests. Profiling stays disabled by default; the config and documentation now say so for both profiles_sample_rate and profiles_sampler. Error reporting and tracing are unaffected. The todo entry records the fix once its dependencies are released: the sentry/sentry per-transaction profiler factory (getsentry/sentry-php#2253) and Excimer's sample filter (https://gerrit.wikimedia.org/r/c/mediawiki/php/excimer/+/1354126), with the attribution, lifecycle, cleanup and overhead checks needed before the warning is removed. Verified with composer lint:fix.
The service provider set the pooled transport directly on the client builder, which takes precedence over the SDK's transport option, so a transport passed through the configuration or set on the builder's options was ignored. The pool also built its own HTTP clients, so the http_client option, a client set on the builder, and the builder's logger never reached the pooled transports. The pooled transport is now the default transport option, installed only when none is configured, so the SDK's precedence applies: a transport set on the builder or in the options replaces it. Pooled transports are created on first use from the customized builder's options, HTTP client and logger; without a configured client, each transport still gets its own SDK client identified as the Hypervel SDK. The pool settings are only read and validated when the pool is used. Sends without a DSN or Spotlight are skipped before borrowing a transport or starting a delivery coroutine, as the SDK transport would skip them anyway. Spotlight-only sends are still delivered asynchronously. The documentation now says to set a custom transport on the builder, since objects in config/sentry.php prevent config caching, and that a custom HTTP client keeps asynchronous delivery. Verified with the Sentry suite under ParaTest, composer lint:fix and composer analyse, plus mutation checks for the default-transport guard, the early skip, the Spotlight path, options-level installation and the builder's HTTP client.
When a coroutine test threw while child coroutines it started were still blocked, the runner waited for those children until the test's time limit and then reported only "This test was aborted after N seconds". The assertion failure or error that caused the problem was lost. If the test method returned or threw before the deadline and its children are what reached the limit, the runner now rethrows the test's own exception. Failures and errors are reported with their original message. An exception the test expected would otherwise make PHPUnit pass the test, so the runner keeps it and a post-condition fails the test, naming the leftover children with the expected exception as the cause. Skipped and incomplete tests never reach post-conditions, so they keep the time-limit abort. Passing tests that leave children running still time out, and children are not cancelled earlier than before. The time-limit fixture covers failures, errors, expected exceptions and assertion failures, and skipped and incomplete tests, each with children still running at the limit, and checks that tearDown() still runs. Expected-exception tests whose children finish in time still pass, with the children completing normally. Verified with TimeLimitTest, composer lint:fix, composer analyse and the full composer test:parallel suite, plus mutation checks that disabling the rethrow or the skipped/incomplete exclusion fails the new cases.
Six Sentry tests start child coroutines that wait on a channel until the test releases them. The releases ran after the test's assertions, so a failed assertion left the children blocked until the time limit. The releases, and the joins where the test waits for the children, now run in finally blocks, so a failure is reported straight away and the children finish. Verified with the five affected test files, composer lint:fix and the full composer test:parallel suite.
Integrate the latest HTTP transport reuse, response-failure handling, database date-format caching, coroutine cleanup and provider configuration changes. Retain the shared Guzzle rejection-response helper with the exception import required by the new fake transport paths. Combine Guzzle compatibility coverage and preserve filesystem wrapping coverage through the real Sentry decorator without duplicating the manager test. Validated with 44,238 full-suite tests, 1,218 focused integration tests, 93 filesystem tests, formatting and both static-analysis passes. Guzzle 7 CI retains destination-policy coverage and reports the known native Swoole failure without exclusions.
The coroutine test failure-reporting fix is preserved as f40df6c on upstream-sync-framework-19. Remove it from the Sentry change set so it can be reviewed and published with the independent framework fixes. Keep the Sentry tests' exception-safe child cleanup here. Restore the three runner files exactly to the current 0.4 versions; no Sentry source changes are included.
The server-process provider registered its start and stop loggers as event observers, but typed their callbacks to receive the event object. The dispatcher calls observers with the event name and payload, so every BeforeProcessHandle dispatch threw a TypeError inside AbstractProcess::bind() before the process's handle() method ran, and every AfterProcessHandle dispatch failed the same way. Accept the observer arguments and pass the event from the payload to each listener. Add a regression test that dispatches both events through a real dispatcher and asserts both log lines. It fails with the TypeError before the fix and passes after it. Formatting, static analysis and the ServerProcess and Signal suites pass.
The server process boots the application once, then forks every worker and process. Two pieces of logging state did not survive that model. A deprecation raised while the server booted wrote a deprecations channel copied from the boot-time configuration. Workers rebuild configuration from their own environment, but the copied channel was never recorded for replay, so they lost it or kept the server's driver. Record the derivation through the configuration mutation tracker so each worker derives the channel again from its rebuilt configuration, and keep an explicitly configured channel. The tracker and ServiceProvider::configureUsing() now accept the configuration contract, since neither needs the concrete repository. Log channels resolved during boot were inherited by every forked process. Buffered records were written once per process, and workers shared the server's handlers and their open resources. Before the server forks, close each resolved Monolog channel so buffered records are written once, then forget every channel so workers create their own. Add regression tests for deprecation channel replay and for closing and forgetting channels before the fork, including buffered stacks and custom loggers without a Monolog lifecycle. Each fails before the fix. Formatting, static analysis and the Foundation, Support, Log, Integration/Log and Testbench exception suites pass.
The server process boots the application, then forks every worker and process from that state. With the SDK's default cURL share handle, connections the server opened while booting were kept in it and reused by every forked process at once. Until a forked worker or custom process starts, the server process now sends without connection sharing. Each forked process then restores the setting. This runs after builder extenders, so an application that disables sharing keeps it disabled, and one that enables it gets it back after the fork. Console processes are unchanged. Events and logs captured while the server booted were inherited and sent by every forked process. Send them once before the server forks. This runs after the log provider has closed buffered channels, so their records are included. The Sentry log channels were written into configuration from boot-time values and replayed literally into workers. Compute them through configureUsing() so each worker derives them from its rebuilt configuration, keeping channels the application defines. Add regression tests for the pre-fork send, including buffered log channels and a later flush that sends nothing again; for connection sharing across worker and custom-process starts and the application's chosen setting; for console processes; and for log channel replay. Each fails before the fix. Formatting, static analysis and the Sentry, OpenTelemetry, Log, Integration/Log, ServerProcess and Signal suites pass.
Bring in the merged Inertia, Slack, filesystem pooling and worker-lifecycle fixes while preserving Sentry instrumentation and coroutine-specific cleanup. The merge applies without conflicts and retains both sides of the test-state cleanup changes. Validated the combined tree with the Sentry, Telescope, Filesystem, Inertia and PHPUnit-support tests, plus source and type static analysis.
Preserve the Sentry assessment harness under tests/Benchmarks with portable application setup, explicit smoke and measurement modes, and documented baseline comparisons. Keep generated applications and raw output outside the repository. Cover HTTP instrumentation, transport saturation, request fetching, Guzzle spans and coroutine propagation. Own subprocess lifetimes, reject occupied ports, and fail runs when response or telemetry checks disagree. Validate both revisions with HTTP and component smoke checks, instrumentation inventories, failure cleanup, PHPUnit discovery and PHP/shell syntax checks. Full performance measurements remain pending an idle-machine run.
Build PSR-7 headers together, retain the invalid-header fallback, and preserve numeric header names. Use direct comparisons for fixed sensitive cookie names and avoid repeated context lookups and array copies in Hub access. Verify header preservation and cookie filtering alongside the complete Sentry suite and static analysis.
Quote the null channel name and verify cached configuration. Disable OPcache file-update protection for the freshly generated immutable benchmark files so workers measure cached views rather than repeated compilation. Record runtime flags and a final garbage-collected resource snapshot outside timed workloads to distinguish retained state from collectible cycles.
Document paired HTTP and component results, request-collection optimizations, disabled instrumentation, pending Guzzle memory, and bounded transport behavior under saturation. Include absolute CPU costs, observed variation, runtime settings, and the corrected benchmark setup.
After recording both saturated waves and resource snapshots, remove the sink delay and require one error and one transaction from a single request on the same server. This catches a closed transport pool even when its counters have drained. Inject the fixture controller services and distinguish historical timing results from these smoke-tested corrections. Verify normal recovery on both revisions and failure with a deliberately closed pool.
Bring in the Swoole 6.3.0 RC1 requirement and current runtime regressions while preserving the Sentry SDK minimum and expanded Guzzle compatibility coverage. Verified the conflict-free merge with refreshed dependencies, formatting, source and type-fixture analysis, and the affected Sentry, HTTP, Telescope, DI, filesystem and lifecycle suites. A native AIO shutdown hang was reproduced independently and reported as swoole/swoole-src#6297; the affected suite passed on a subsequent run.
Refresh the Sentry snapshot verification date after the reviewed optimization work and current 0.4 merge. Track adoption of the released Swoole shutdown fix without adding a local workaround or an unreliable race test.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Member
Author
|
@coderabbitai review |
|
Member
Author
|
@cubic-dev-ai review |
@binaryfire I have started the AI code review. It will take a few minutes to complete. |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
192
to
+199
| 'features' => [ | ||
| CacheFeature::class, | ||
| QueueFeature::class, | ||
| NotificationsFeature::class, | ||
| LogFeature::class, | ||
| LogIntegration::class, | ||
| CacheIntegration::class, | ||
| QueueIntegration::class, | ||
| ConsoleIntegration::class, | ||
| ConsoleSchedulingFeature::class, | ||
| RedisFeature::class, | ||
| StorageIntegration::class, | ||
| ], | ||
|
|
||
| // Exceptions that should not be reported to Sentry | ||
| 'ignore_exceptions' => [ | ||
| ValidationException::class, | ||
| NotificationsIntegration::class, | ||
| ConsoleSchedulingIntegration::class, |
There was a problem hiding this comment.
Applications that already ran sentry:publish keep the deleted CacheFeature, QueueFeature, LogFeature, NotificationsFeature, ConsoleSchedulingFeature, and RedisFeature names in their features list. That list replaces these new defaults. registerFeatures() and bootFeatures() catch the missing-class errors and skip the affected features, removing cache, queue, notification, and scheduled-task telemetry. The Sentry log drivers are also no longer registered.
Preserve compatible wrappers for the old names, or translate old feature lists before resolving them. Keep Redis's independent switch when translating RedisFeature.
Member
Author
There was a problem hiding this comment.
The 0.4 line is unreleased and intentionally breaking. These feature names are being aligned before its first release, so we are not adding compatibility wrappers or translation for older development snapshots. The shipped feature list contains the current classes.
There was a problem hiding this comment.
That’s fair. Since the 0.4 line is unreleased and intentionally breaking, applications from development snapshots are not a supported compatibility target. The feature list is correctly aligned to the current integration class names, so this finding does not apply; no compatibility wrappers or translation are needed.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
19 issues found across 170 files
Confidence score: 3/5
FetchesStackTrace.phpcalls an API missing from somehypervel/diversions allowed by Telescope’s^0.4constraint, so trace collection can fail at runtime. Raise the minimum DI version.ClientRequestWatcher.phpcan turn a successful HTTP response into a rejected promise if recording fails. Catch recording exceptions in both settlement handlers.LogServiceProvider.phpcloses handlers without updating channel objects that long-lived services already hold, so those services can keep logging through pre-fork handlers. Refresh or replace retained channels after the fork.GuzzleHttpClientAspect.phpcan add a breadcrumb to the wrong Sentry scope when a promise settles in another coroutine. Capture the originating hub or scope and record the breadcrumb there.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/telescope/src/Watchers/FetchesStackTrace.php">
<violation number="1" location="src/telescope/src/Watchers/FetchesStackTrace.php:27">
P2: This call requires an API that older `hypervel/di` releases allowed by Telescope's `^0.4` constraint do not provide, causing trace collection to fail with an undefined-method error. Raise the minimum DI version to the release adding `proxySources()` or retain compatibility with older versions.</violation>
</file>
<file name="tests/Sentry/ServiceProviderWithoutDsnTest.php">
<violation number="1" location="tests/Sentry/ServiceProviderWithoutDsnTest.php:60">
P3: These tests do not depend on `testIsBound` or its output, so the dependency can skip them when the producer is filtered out and unnecessarily couples their results. Remove the `Depends` attributes from all three tests.</violation>
</file>
<file name="docs/upstream-sync/sync.yaml">
<violation number="1" location="docs/upstream-sync/sync.yaml:278">
P2: This drops the dedicated `getsentry/sentry-docs` checkpoint, so applicable Sentry documentation updates will no longer be tracked. Restore that entry alongside the code-package entry.</violation>
</file>
<file name="src/telescope/src/Watchers/ClientRequestWatcher.php">
<violation number="1" location="src/telescope/src/Watchers/ClientRequestWatcher.php:86">
P2: A `recordTransfer()` failure here turns a successful HTTP response into a rejected promise; `Telescope::recordClientRequest()` can propagate exceptions from `afterRecordingHook`. Catch recording failures in both settlement handlers so Telescope cannot change the request result.</violation>
</file>
<file name="tests/Benchmarks/Sentry/Fixtures/app/app/Bench/StatsCallback.php">
<violation number="1" location="tests/Benchmarks/Sentry/Fixtures/app/app/Bench/StatsCallback.php:39">
P2: The stats endpoint fatals on PHP builds without the OPcache extension because both calls to `opcache_get_status()` are unconditional. Guard them with `function_exists()` or make OPcache an explicit benchmark prerequisite.</violation>
</file>
<file name="tests/Sentry/Http/HypervelRequestFetcherTest.php">
<violation number="1" location="tests/Sentry/Http/HypervelRequestFetcherTest.php:195">
P2: This helper leaves the request in `RequestContext` after fetching it, so later tests without a current request can inherit stale HTTP data. Forget the context in a `finally` block after the fetch.</violation>
</file>
<file name="src/sentry/src/SentryServiceProvider.php">
<violation number="1" location="src/sentry/src/SentryServiceProvider.php:45">
P2: This provider now depends on `hypervel/server-process` for its `BeforeProcessHandle` listener, but `src/sentry/composer.json` does not declare that runtime dependency. Add it to this package's `require` section so standalone installs include the fork-lifecycle integration.
(Based on your team's feedback about declaring split-package runtime dependencies.)</violation>
<violation number="2" location="src/sentry/src/SentryServiceProvider.php:687">
P2: `registerCoroutineContextPropagation` now stores the parent coroutine's `Request` instance directly (not a clone) in the child's Sentry context. The previous code cloned it. Because the request is shared and this key is not a `ReplicableContext` value, the child can later observe parent-side mutations (body consumption, header/session changes) when it captures an event. Clone the request before storing it to keep per-coroutine isolation.</violation>
</file>
<file name="tests/Benchmarks/Sentry/benchmark.php">
<violation number="1" location="tests/Benchmarks/Sentry/benchmark.php:262">
P2: Shutdown requires ext-posix through `posix_kill()`, but the documented prerequisites list PCNTL only; without ext-posix, cleanup fatals and can leave the benchmark processes running. Use `proc_terminate($process, $signal)` for both shutdown calls or document and check the additional extension.</violation>
</file>
<file name="tests/Benchmarks/Sentry/request-fetcher.php">
<violation number="1" location="tests/Benchmarks/Sentry/request-fetcher.php:70">
P2: `opcache_get_status()` is undefined when the optional OPcache extension is absent, aborting the benchmark before it starts. Guard this metadata check with `function_exists()`.</violation>
<violation number="2" location="tests/Benchmarks/Sentry/request-fetcher.php:108">
P2: The processor timing also includes a fresh `Event::createTransaction()` on every iteration, so `processor_us` does not isolate `processEvent()`. Move event construction outside the timed loop or report a matched construction baseline.</violation>
</file>
<file name="src/log/src/LogServiceProvider.php">
<violation number="1" location="src/log/src/LogServiceProvider.php:40">
P2: Closing these handlers does not update channel objects already returned by `channel()`: long-lived services that retained one keep using the pre-fork logger and bypass newly resolved worker handlers or worker-specific configuration. Rebuild or rebind retained channel references on worker startup before closing the old handlers.</violation>
</file>
<file name="tests/Sentry/Features/HttpClientDataCollectionTest.php">
<violation number="1" location="tests/Sentry/Features/HttpClientDataCollectionTest.php:29">
P2: These assertions cover only a literal `token` key, so they will not catch regressions that leak secrets under decoded or bracketed names, or in malformed and relative URLs. Add those query cases and verify unrelated URL bytes remain unchanged.
(Based on your team's feedback about sensitive URL query redaction.)</violation>
</file>
<file name="src/sentry/src/Aspects/GuzzleHttpClientAspect.php">
<violation number="1" location="src/sentry/src/Aspects/GuzzleHttpClientAspect.php:185">
P2: The three successive `setData()` calls in `recordTransfer` clobber each other: sentry-php's `Span::setData()` replaces the whole data array (which is why the old `finishSpan` merged `$span->getData()` before calling it). The response collector and then the origin collector overwrite the earlier `url`/`method`/`status_code` data, so most HTTP span data never gets sent. Merge with `getData()` before each `setData()` — for example `$span->setData(array_merge($span->getData(), $origin))` for the origin block — or build one combined array and set it once.</violation>
<violation number="2" location="src/sentry/src/Aspects/GuzzleHttpClientAspect.php:210">
P2: A promise settled from another coroutine records this breadcrumb on that coroutine's current Sentry scope, not the scope that created the request. Capture the originating hub or scope and add the breadcrumb there.</violation>
</file>
<file name="src/di/src/Bootstrap/GenerateProxies.php">
<violation number="1" location="src/di/src/Bootstrap/GenerateProxies.php:96">
P2: Regenerating a loaded class into the same proxy path with a different source overwrites the mapping, although PHP keeps the old proxy code loaded. Preserve the existing mapping for loaded classes so exception frames retain the correct vendor/main classification.</violation>
</file>
<file name="src/sentry/src/Features/ConsoleSchedulingIntegration.php">
<violation number="1" location="src/sentry/src/Features/ConsoleSchedulingIntegration.php:242">
P3: In-progress check-ins are stored in the coroutine-local map but, unlike the spans/scopes tracked via TracksPushedScopesAndSpans, there is no deferred cleanup: if a task coroutine ends without finishCheckIn() (e.g., a CanceledException propagates from Event::run() or the scheduler stops mid-task), the in-progress CheckIn stays in the context for the coroutine's lifetime and the monitor is never marked finished on Sentry's side. Since Hypervel's ScheduleRunCommand runs each task in a short-lived per-task coroutine the map is normally dropped with coroutine teardown, but the code documents a deliberately missing cache stage without a safeguard for aborted runs. Consider registering a Coroutine::defer that forgets CHECK_INS_CONTEXT_KEY (or sends an error check-in) when finishCheckIn() wasn't reached.</violation>
</file>
<file name="src/sentry/src/EventHandler.php">
<violation number="1" location="src/sentry/src/EventHandler.php:237">
P3: `beforeServerForkHandler` runs `drainEvents()` synchronously in the master process, and `drainEvents()` waits up to `max(1, http_timeout)` seconds (2s by default) for the transport to deliver. With an unreachable DSN or a saturated pool, every server boot stalls for the full timeout before forking. Bound the flush timeout here (or make it best-effort) so boot is not blocked on Sentry delivery.</violation>
</file>
<file name="src/sentry/src/Features/CacheIntegration.php">
<violation number="1" location="src/sentry/src/Features/CacheIntegration.php:326">
P2: `setData()` replaces the span's data map, so finishing a cache span wipes the `cache.key`/`cache.ttl` data that was set when the span started and keeps only `cache.hit` (and `cache.success` in the `KeyWritten`/`KeyWriteFailed` branch). The `count($finishedSpan->getData()['cache.key'] ...)` check just above reads the keys and then discards them. Merge the flag into the existing data instead so finished spans keep their keys.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
|
||
| return ! Str::contains($frame['file'], $this->ignoredPaths()); | ||
| // A generated AOP proxy runs its source file's code, so the source decides whether the frame is ignored. | ||
| $file = GenerateProxies::proxySources()[$frame['file']] ?? $frame['file']; |
There was a problem hiding this comment.
P2: This call requires an API that older hypervel/di releases allowed by Telescope's ^0.4 constraint do not provide, causing trace collection to fail with an undefined-method error. Raise the minimum DI version to the release adding proxySources() or retain compatibility with older versions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/telescope/src/Watchers/FetchesStackTrace.php, line 27:
<comment>This call requires an API that older `hypervel/di` releases allowed by Telescope's `^0.4` constraint do not provide, causing trace collection to fail with an undefined-method error. Raise the minimum DI version to the release adding `proxySources()` or retain compatibility with older versions.</comment>
<file context>
@@ -22,7 +23,10 @@ protected function getCallerFromStackTrace(array $forgetLines = []): ?array
- return ! Str::contains($frame['file'], $this->ignoredPaths());
+ // A generated AOP proxy runs its source file's code, so the source decides whether the frame is ignored.
+ $file = GenerateProxies::proxySources()[$frame['file']] ?? $frame['file'];
+
+ return ! Str::contains($file, $this->ignoredPaths());
</file context>
Member
Author
There was a problem hiding this comment.
There is no earlier stable 0.4 release to support. The DI API and this consumer will ship together in the first 0.4 release, so the existing ^0.4 constraint is correct.
| getsentry/sentry-laravel: | ||
| branch: master | ||
| checked_through: null | ||
| checked_through: 0537a1d5bfd583beb89001887af9cadd3116bccb |
There was a problem hiding this comment.
P2: This drops the dedicated getsentry/sentry-docs checkpoint, so applicable Sentry documentation updates will no longer be tracked. Restore that entry alongside the code-package entry.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At docs/upstream-sync/sync.yaml, line 278:
<comment>This drops the dedicated `getsentry/sentry-docs` checkpoint, so applicable Sentry documentation updates will no longer be tracked. Restore that entry alongside the code-package entry.</comment>
<file context>
@@ -275,13 +275,7 @@ hyperf/hyperf:
getsentry/sentry-laravel:
branch: master
- checked_through: null
+ checked_through: 0537a1d5bfd583beb89001887af9cadd3116bccb
last_reviewed_pr: null
- sync_date: null
</file context>
Member
Author
There was a problem hiding this comment.
Removing that queue entry is intentional: most changes in sentry-docs do not concern this integration. The Sentry page was reconciled directly, and b6550a3 adds a note to check the relevant Laravel documentation when major features or behavior change.
| } | ||
| return $proceedingJoinPoint->process()->then( | ||
| function (ResponseInterface $response) use ($request, $options, $customTags, &$stats): ResponseInterface { | ||
| $this->recordTransfer($request, $stats, $response, $options, $customTags); |
There was a problem hiding this comment.
P2: A recordTransfer() failure here turns a successful HTTP response into a rejected promise; Telescope::recordClientRequest() can propagate exceptions from afterRecordingHook. Catch recording failures in both settlement handlers so Telescope cannot change the request result.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/telescope/src/Watchers/ClientRequestWatcher.php, line 86:
<comment>A `recordTransfer()` failure here turns a successful HTTP response into a rejected promise; `Telescope::recordClientRequest()` can propagate exceptions from `afterRecordingHook`. Catch recording failures in both settlement handlers so Telescope cannot change the request result.</comment>
<file context>
@@ -67,88 +68,69 @@ public function recordRequest(ProceedingJoinPoint $proceedingJoinPoint): mixed
- }
+ return $proceedingJoinPoint->process()->then(
+ function (ResponseInterface $response) use ($request, $options, $customTags, &$stats): ResponseInterface {
+ $this->recordTransfer($request, $stats, $response, $options, $customTags);
- return Create::rejectionFor($reason);
</file context>
Member
Author
There was a problem hiding this comment.
Keeping the existing Telescope contract: recording hooks and filters propagate application exceptions. The upstream watcher does so too. A blanket catch would suppress those failures; the settlement-path tests explicitly preserve this behavior.
Comment on lines
+39
to
+40
| 'enabled' => (opcache_get_status(false) ?: [])['opcache_enabled'] ?? false, | ||
| 'jit' => (opcache_get_status(false) ?: [])['jit']['on'] ?? false, |
There was a problem hiding this comment.
P2: The stats endpoint fatals on PHP builds without the OPcache extension because both calls to opcache_get_status() are unconditional. Guard them with function_exists() or make OPcache an explicit benchmark prerequisite.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tests/Benchmarks/Sentry/Fixtures/app/app/Bench/StatsCallback.php, line 39:
<comment>The stats endpoint fatals on PHP builds without the OPcache extension because both calls to `opcache_get_status()` are unconditional. Guard them with `function_exists()` or make OPcache an explicit benchmark prerequisite.</comment>
<file context>
@@ -0,0 +1,93 @@
+ 'coroutines' => Coroutine::stats()['coroutine_num'],
+ 'gc' => gc_status(),
+ 'opcache' => [
+ 'enabled' => (opcache_get_status(false) ?: [])['opcache_enabled'] ?? false,
+ 'jit' => (opcache_get_status(false) ?: [])['jit']['on'] ?? false,
+ ],
</file context>
Suggested change
| 'enabled' => (opcache_get_status(false) ?: [])['opcache_enabled'] ?? false, | |
| 'jit' => (opcache_get_status(false) ?: [])['jit']['on'] ?? false, | |
| 'enabled' => function_exists('opcache_get_status') ? ((opcache_get_status(false) ?: [])['opcache_enabled'] ?? false) : false, | |
| 'jit' => function_exists('opcache_get_status') ? ((opcache_get_status(false) ?: [])['jit']['on'] ?? false) : false, |
Member
Author
There was a problem hiding this comment.
Documented OPcache as a benchmark prerequisite in b6550a3. The harness requires it for its runtime metadata; it does not need a second execution mode for builds without that extension.
| $this->assertTrue(app()->bound('sentry')); | ||
| } | ||
|
|
||
| #[Depends('testIsBound')] |
There was a problem hiding this comment.
P3: These tests do not depend on testIsBound or its output, so the dependency can skip them when the producer is filtered out and unnecessarily couples their results. Remove the Depends attributes from all three tests.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tests/Sentry/ServiceProviderWithoutDsnTest.php, line 60:
<comment>These tests do not depend on `testIsBound` or its output, so the dependency can skip them when the producer is filtered out and unnecessarily couples their results. Remove the `Depends` attributes from all three tests.</comment>
<file context>
@@ -50,11 +57,13 @@ public function testIsBound(): void
$this->assertTrue(app()->bound('sentry'));
}
+ #[Depends('testIsBound')]
public function testDsnIsNotSet(): void
{
</file context>
Member
Author
There was a problem hiding this comment.
These attributes preserve the upstream tests prerequisite: the integration must be bound before its configuration is inspected. They do not transfer state between tests. Filtering out a declared prerequisite is expected PHPUnit behavior, not a reason to remove applicable upstream coverage.
| // leave it. The application log context would carry it into queued jobs and subprocesses. | ||
| $checkIns = CoroutineContext::get(self::CHECK_INS_CONTEXT_KEY, []); | ||
| $checkIns[$cacheKey] = $checkIn; | ||
| CoroutineContext::set(self::CHECK_INS_CONTEXT_KEY, $checkIns); |
There was a problem hiding this comment.
P3: In-progress check-ins are stored in the coroutine-local map but, unlike the spans/scopes tracked via TracksPushedScopesAndSpans, there is no deferred cleanup: if a task coroutine ends without finishCheckIn() (e.g., a CanceledException propagates from Event::run() or the scheduler stops mid-task), the in-progress CheckIn stays in the context for the coroutine's lifetime and the monitor is never marked finished on Sentry's side. Since Hypervel's ScheduleRunCommand runs each task in a short-lived per-task coroutine the map is normally dropped with coroutine teardown, but the code documents a deliberately missing cache stage without a safeguard for aborted runs. Consider registering a Coroutine::defer that forgets CHECK_INS_CONTEXT_KEY (or sends an error check-in) when finishCheckIn() wasn't reached.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/sentry/src/Features/ConsoleSchedulingIntegration.php, line 242:
<comment>In-progress check-ins are stored in the coroutine-local map but, unlike the spans/scopes tracked via TracksPushedScopesAndSpans, there is no deferred cleanup: if a task coroutine ends without finishCheckIn() (e.g., a CanceledException propagates from Event::run() or the scheduler stops mid-task), the in-progress CheckIn stays in the context for the coroutine's lifetime and the monitor is never marked finished on Sentry's side. Since Hypervel's ScheduleRunCommand runs each task in a short-lived per-task coroutine the map is normally dropped with coroutine teardown, but the code documents a deliberately missing cache stage without a safeguard for aborted runs. Consider registering a Coroutine::defer that forgets CHECK_INS_CONTEXT_KEY (or sends an error check-in) when finishCheckIn() wasn't reached.</comment>
<file context>
@@ -212,11 +235,18 @@ private function startCheckIn(
+ // leave it. The application log context would carry it into queued jobs and subprocesses.
+ $checkIns = CoroutineContext::get(self::CHECK_INS_CONTEXT_KEY, []);
+ $checkIns[$cacheKey] = $checkIn;
+ CoroutineContext::set(self::CHECK_INS_CONTEXT_KEY, $checkIns);
$this->sendCheckIn($checkIn);
</file context>
Member
Author
There was a problem hiding this comment.
The check-in map is destroyed with its task coroutine, so an additional defer to forget it would duplicate cleanup. An interrupted task does not guarantee a final check-in; retaining an unfinished monitor is different from leaking worker state. No reliable-delivery or extra shutdown mechanism is being added here.
| */ | ||
| protected function beforeServerForkHandler(BeforeServerFork $event): void | ||
| { | ||
| Integration::drainEvents(); |
There was a problem hiding this comment.
P3: beforeServerForkHandler runs drainEvents() synchronously in the master process, and drainEvents() waits up to max(1, http_timeout) seconds (2s by default) for the transport to deliver. With an unreachable DSN or a saturated pool, every server boot stalls for the full timeout before forking. Bound the flush timeout here (or make it best-effort) so boot is not blocked on Sentry delivery.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/sentry/src/EventHandler.php, line 237:
<comment>`beforeServerForkHandler` runs `drainEvents()` synchronously in the master process, and `drainEvents()` waits up to `max(1, http_timeout)` seconds (2s by default) for the transport to deliver. With an unreachable DSN or a saturated pool, every server boot stalls for the full timeout before forking. Bound the flush timeout here (or make it best-effort) so boot is not blocked on Sentry delivery.</comment>
<file context>
@@ -204,6 +227,16 @@ protected function messageLoggedHandler(LogEvents\MessageLogged $logEntry): void
+ */
+ protected function beforeServerForkHandler(BeforeServerFork $event): void
+ {
+ Integration::drainEvents();
+ }
+
</file context>
Member
Author
There was a problem hiding this comment.
The drain is already bounded by the configured HTTP timeout and returns immediately when no sends are pending. It runs once before forking to avoid duplicating buffered telemetry in children. A second timeout setting would not fix a missing bound; that bound already exists.
Count expected terminal events within each coroutine so a bulk read or write does not finish its span after the first key and accidentally pop an outer operation. Match deduplicated read results, per-key write results and whole-read failures. Document the intentional request-local tracing limitation when a cache listener throws and its caller continues. Cover nested failover reads and writes, repeated keys, failure completion and parent restoration. Validation: focused regressions fail before the fix; the Sentry suite, formatting and source/type analysis pass.
Check the SDK body policy before constructing the PSR-7 request. Preserve the distinct legacy and data-collection length rules, including unknown lengths and unlimited legacy bodies. Reuse eligible content for header fallback and legacy JSON parsing instead of reading a resource twice. Cover excluded and eligible resource-backed bodies while retaining event, transaction, form and upload coverage. This avoids excluded reads without introducing a streaming abstraction or changing collection limits. Validation: the excluded-body probe fails before the fix; focused fetcher tests, the Sentry suite, formatting and source/type analysis pass.
Verify the actual exception captured by console tests and isolate shipped-default assertions from ambient environment overrides. Describe the full batch included in log context and document benchmark extension prerequisites and event allocation in processor measurements. Direct future integration updates to the relevant Sentry Laravel documentation when major features or behavior change, without tracking the unrelated documentation queue. Validation: configuration tests reproduce the environment-dependent failure before the fix; affected test files, the full Sentry suite, formatting and source/type analysis pass.
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
1 participant
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.
This improves Sentry's data collection, tracing and behavior in long-lived workers. It brings the integration up to date with the released PHP SDK, fixes missing or incorrectly attributed telemetry, and reduces repeated work in request collection and scope access.
Data collection and configuration
sentry/sentry ^4.34and support itsdata_collectionpolicy across requests, responses, user information, database queries, Redis commands, queued jobs and console input. Leaving the option unset preserves the existing collection settings. Command arguments and options are filtered by name instead of exposing the raw command line under the new policy.default_integrations, callable integration overrides, custom transports, HTTP clients and loggers. The asynchronous pool is the default transport; explicit SDK configuration takes precedence. A send without a DSN or Spotlight returns before borrowing a transport or starting a coroutine.CacheIntegrationwhile retaining independent switches. Disabled features avoid their recording work; inactive Sentry registers no instrumentation listeners, middleware, Guzzle aspect, coroutine propagation hook or Blade decorator.sentry:publishhandling of exported, spaced, multiline and empty dotenv assignments. An empty DSN no longer skips the prompt. Explicitly captured validation exceptions follow the application'signore_exceptionssetting; the framework exception handler still excludes them from automatic reporting.Correct tracing and coroutine lifetimes
on_statscallback still runs on every hop, and rejections pass through unchanged. This covers the HTTP client, direct Guzzle clients and SDKs built on Guzzle.Worker startup and related fixes
Sentry sends boot-time events and buffered logs once before the server forks. The master does not keep SDK shared connections for its workers to inherit, and workers restore the configured sharing behavior. Resolved logging channels are closed and forgotten before the fork so each worker creates its own handlers.
Sentry log channels and the deprecation channel are derived again from each worker's rebuilt configuration. Previously, literal copies of boot-time settings could overwrite the worker's settings. Explicit application channels remain intact.
The same HTTP completion correction applies to Telescope's client-request watcher. Sentry, Telescope and the HTTP client share rejected-response extraction for Guzzle 7 and 8. The exception renderer and Telescope also use the AOP source mapping. Process lifecycle log observers now accept the dispatcher's event-name/payload arguments rather than throwing before the process runs.
Performance and resource usage
The retained optimizations use direct cookie-name comparisons, construct valid PSR-7 headers together instead of cloning per header, and avoid repeated coroutine-context lookups and scope-stack copies. They add no request cache or worker-lifetime lookup registry.
Request collection, in microseconds per call:
data_collectionGETdata_collectionJSON POST, about 2 KBAn isolated
Hub::getSpan()comparison measured 0.446 → 0.210 µs. Copying a populated scope into a child coroutine measured 5.49 → 2.96 µs between the original and final packages.For the fully sampled mixed HTTP workload at concurrency 32:
The CPU reduction is 1.6%. These latency figures include queueing in a single worker driven to capacity. Small differences in other configurations were within run-to-run variation. Fully sampled tracing still has a substantial absolute cost: in this mixed fixture it adds about 1,484 µs CPU per request over excluding Sentry, while errors-only adds about 267 µs. Sampling remains important at high traffic levels.
Correct outgoing HTTP observation costs more than the previous incomplete recording:
The approximately 12 µs increase works out to 0.06 CPU cores at 5,000 instrumented calls/sec and 0.12 at 10,000 calls/sec. These are calculations from isolated timings, not measured throughput at those rates, and exclude transaction serialization and delivery. The extra memory is about 7.34 KiB per outstanding sampled request, released when that work finishes. With 100 ms average duration, those rates imply about 3.6 MiB and 7.2 MiB extra pending memory respectively. Disabling origin capture avoids about 3.67 KiB per pending request in this fixture while preserving completion recording.
Removing the completion observer would lose final responses, redirects and rejection information. Capturing async origins at completion cannot recover a caller that has already returned. Both paths remain; synchronous origins are threshold-gated and origin capture can be disabled.
Transport saturation stayed bounded: after repeated waves, both revisions returned to ten idle connections with no borrowed connections, waiting borrowers or pending sends. Every application request succeeded. The deliberately slow sink received only about 11% of attempted telemetry on both revisions; this is a bounded transport, not a lossless queue. Separate checks verify that delivery resumes after saturation.
Measurements used an idle six-vCPU machine, PHP 8.4.25, Swoole 6.3.0RC1, matching dependencies, independent production-only apps, OPcache enabled and JIT disabled. Tables use medians of three alternating process pairs. The committed harness and results include source revisions, workloads, variation, memory checks and measurement limits. Later fixture dependency-injection and post-saturation checks were smoke-tested; the historical timings are identified separately. The tables also predate the bulk-cache counter and excluded-body read corrections, which were verified with regression tests.
Documentation and verification
Update the Sentry integration guide, Laravel porting guidance, package differences and upstream tracking notes. The guide covers configuration, instrumentation, OTLP correlation, child coroutines and transport behavior, and links to Sentry's documentation for general SDK usage.
Profiling remains disabled by default. The documentation warns that the current profiler can attribute other coroutines' work to a transaction; adoption of the proposed released SDK/Excimer APIs is tracked separately. This does not affect error reporting or tracing.
Reconcile applicable upstream source and test coverage, preserving Hypervel's coroutine and transport tests. Add regression coverage for the corrected lifetimes, configuration, request data, redirects, failures and cleanup. Validation includes affected package suites, formatting and source/type-fixture analysis, with Guzzle 7 and 8 compatibility coverage in CI.
Note
Improve Sentry instrumentation with data-collection policies and coroutine isolation
send_default_piibehavior with a configurabledata_collectionpolicy in sentry.php. The policy now governs user info, cookies, headers, request/response bodies, SQL query parameters, queue payloads, and console command input, with sensitive-value filtering throughout.CoroutineContextfor coroutine isolation: queue jobs use nested job frames, scheduled check-ins store their state per coroutine, and notification spans use an owner stack so terminal events only finish the matching span. See QueueIntegration.php and NotificationsIntegration.php.ClientBuilderin HttpTransportPool.php.QueueFeature→QueueIntegration,LogFeature→LogIntegration, etc.),send_default_piiis reported as IGNORED when data collection is configured, the Sentry SDK constraint moves from^4.32to^4.34, andsymfony/psr-http-message-bridgeis replaced byvlucas/phpdotenvin composer.json.Macroscope summarized b6550a3.