-
-
Notifications
You must be signed in to change notification settings - Fork 28
fix(sentry): stop flushing metrics on every request #1081
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,8 +49,6 @@ public function process(ProceedingJoinPoint $proceedingJoinPoint): mixed | |
| 'class' => $proceedingJoinPoint->className, | ||
| 'method' => $proceedingJoinPoint->methodName, | ||
| ]); | ||
|
Comment on lines
49
to
51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When AGENTS.md reference: AGENTS.md:L122-L125 Useful? React with 👍 / 👎. |
||
|
|
||
| metrics()->flush(); | ||
| } | ||
|
|
||
| return $proceedingJoinPoint->process(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,7 +62,7 @@ public function process(object $event): void | |
| ++$this->stats->response_count; | ||
| --$this->stats->connection_num; | ||
|
|
||
| $timer->end(true); | ||
| $timer->end(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When metrics are enabled but Useful? React with 👍 / 👎. |
||
|
|
||
| unset($timer); | ||
| }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
| /** | ||
| * This file is part of friendsofhyperf/components. | ||
| * | ||
| * @link https://github.com/friendsofhyperf/components | ||
| * @document https://github.com/friendsofhyperf/components/blob/main/README.md | ||
| * @contact huangdijia@gmail.com | ||
| */ | ||
|
|
||
| namespace FriendsOfHyperf\Tests\Sentry\Metrics; | ||
|
|
||
| use FriendsOfHyperf\Sentry\Feature; | ||
| use FriendsOfHyperf\Sentry\Metrics\CoroutineServerStats; | ||
| use FriendsOfHyperf\Sentry\Metrics\Listener\RequestWatcher; | ||
| use Hyperf\HttpMessage\Server\Request; | ||
| use Hyperf\HttpServer\Event\RequestReceived; | ||
| use Mockery as m; | ||
|
|
||
| function waitForCoroutine(int $cid): void | ||
| { | ||
| if (\Swoole\Coroutine::getCid() === -1) { | ||
| return; // Top level: the created coroutine already finished synchronously. | ||
| } | ||
|
|
||
| while (\Swoole\Coroutine::exists($cid)) { | ||
| \Swoole\Coroutine::sleep(0.001); | ||
| } | ||
| } | ||
|
|
||
| test('process request received increments counters without throwing', function () { | ||
| $stats = new CoroutineServerStats(); | ||
| $feature = m::mock(Feature::class); | ||
| $feature->shouldReceive('isMetricsEnabled')->andReturn(true); | ||
|
|
||
| $watcher = new RequestWatcher($stats, $feature); | ||
| $request = new Request('GET', 'http://127.0.0.1:9501/health'); | ||
|
|
||
| $snapshot = null; | ||
| $cid = \Swoole\Coroutine::create(function () use ($watcher, $request, $stats, &$snapshot) { | ||
| $watcher->process(new RequestReceived($request, null)); | ||
| $snapshot = [ | ||
| 'accept_count' => $stats->accept_count, | ||
| 'request_count' => $stats->request_count, | ||
| 'connection_num' => $stats->connection_num, | ||
| ]; | ||
| }); | ||
|
|
||
| waitForCoroutine($cid); | ||
|
|
||
| expect(\Swoole\Coroutine::exists($cid))->toBeFalse() | ||
| ->and($snapshot['accept_count'])->toBe(1) | ||
| ->and($snapshot['request_count'])->toBe(1) | ||
| ->and($snapshot['connection_num'])->toBe(1); | ||
| }); | ||
|
|
||
| test('defer closes the request counters after the coroutine ends', function () { | ||
| $stats = new CoroutineServerStats(); | ||
| $feature = m::mock(Feature::class); | ||
| $feature->shouldReceive('isMetricsEnabled')->andReturn(true); | ||
|
|
||
| $watcher = new RequestWatcher($stats, $feature); | ||
| $request = new Request('GET', 'http://127.0.0.1:9501/health'); | ||
|
|
||
| $cid = \Swoole\Coroutine::create(function () use ($watcher, $request) { | ||
| $watcher->process(new RequestReceived($request, null)); | ||
| }); | ||
|
|
||
| waitForCoroutine($cid); | ||
|
|
||
| expect($stats->close_count)->toBe(1) | ||
| ->and($stats->response_count)->toBe(1) | ||
| ->and($stats->connection_num)->toBe(0); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new buffering and reporting behavior is documented only in the publishable configuration; neither component README nor any of the four localized Sentry pages was updated. The repository specifically requires all four Sentry pages and both READMEs to remain semantically synchronized for metrics changes, so mirror the verified lifecycle guidance across those sources.
AGENTS.md reference: AGENTS.md:L129-L130
Useful? React with 👍 / 👎.