Sentry package #222
Sentry package #222
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces comprehensive Sentry integration to the Hypervel framework, adding error tracking and performance monitoring capabilities. The implementation includes a new Sentry package with features for tracking cache operations, database queries, queue jobs, notifications, and console scheduling.
- Adds complete Sentry SDK integration with custom Hub implementation for coroutine support
- Implements feature-based architecture for monitoring different framework components
- Includes comprehensive test coverage for all Sentry features
Reviewed Changes
Copilot reviewed 64 out of 64 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/sentry/ | New Sentry package with comprehensive error tracking and performance monitoring |
| tests/Sentry/ | Test suite covering all Sentry features and integrations |
| tests/Cache/ | Updated cache event tests to include new event types |
| src/cache/ | Enhanced cache events with additional event types for better monitoring |
| src/foundation/ | Updated session interaction test helper |
| composer.json | Added Sentry dependency and package registration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| use Sentry\Monolog\LogsHandler; | ||
|
|
There was a problem hiding this comment.
Import mismatch - the class imports Sentry\Monolog\LogsHandler but uses a local LogsHandler class. This should import the local class Hypervel\Sentry\Logs\LogsHandler instead.
| use Sentry\Monolog\LogsHandler; |
Copilot uses AI. Check for mistakes.
| use Sentry\Monolog\LogsHandler; | ||
|
|
||
| class LogChannel extends LogManager | ||
| { | ||
| public function __invoke(array $config = []): Logger | ||
| { | ||
| $handler = new LogsHandler( |
There was a problem hiding this comment.
The handler instantiation uses LogsHandler but should use the correct handler class. Based on the import and usage pattern, this should be new \Hypervel\Sentry\Logs\LogsHandler() to match the local implementation.
| use Sentry\Monolog\LogsHandler; | |
| class LogChannel extends LogManager | |
| { | |
| public function __invoke(array $config = []): Logger | |
| { | |
| $handler = new LogsHandler( | |
| // use Sentry\Monolog\LogsHandler; // Removed: use local LogsHandler instead | |
| class LogChannel extends LogManager | |
| { | |
| public function __invoke(array $config = []): Logger | |
| { | |
| $handler = new \Hypervel\Sentry\Logs\LogsHandler( |
Copilot uses AI. Check for mistakes.
| private $message; | ||
|
|
||
| public function __construct($message = null) |
There was a problem hiding this comment.
The property $message should have proper type declaration. It should be declared as private ?string $message; and the constructor parameter should be typed as ?string $message = null.
| private $message; | |
| public function __construct($message = null) | |
| private ?string $message; | |
| public function __construct(?string $message = null) |
Copilot uses AI. Check for mistakes.
| private $message; | ||
|
|
||
| public function __construct($message = null) |
There was a problem hiding this comment.
The property $message should have proper type declaration. It should be declared as private ?string $message; and the constructor parameter should be typed as ?string $message = null.
| private $message; | |
| public function __construct($message = null) | |
| private ?string $message; | |
| public function __construct(?string $message = null) |
Copilot uses AI. Check for mistakes.
| \sprintf( | ||
| 'Transaction [%s] was started but tracing is not enabled.', | ||
| (string) $transaction->getTraceId() | ||
| ), |
There was a problem hiding this comment.
[nitpick] Use the global sprintf function instead of \sprintf for consistency with the rest of the codebase. The leading backslash is unnecessary here.
Copilot uses AI. Check for mistakes.
| \sprintf( | ||
| 'Transaction [%s] was started but not sampled because sample rate (decided by %s) is invalid.', | ||
| (string) $transaction->getTraceId(), | ||
| $sampleSource | ||
| ), |
There was a problem hiding this comment.
[nitpick] Use the global sprintf function instead of \sprintf for consistency. This pattern appears multiple times in the file and should be addressed consistently.
Copilot uses AI. Check for mistakes.
| @@ -37,8 +38,8 @@ public function session(array $data): static | |||
| */ | |||
| protected function startSession(): static | |||
| { | |||
| if (! $this->app->get(SessionContract::class)->isStarted()) { | |||
| $this->app->get(SessionContract::class)->start(); | |||
| if (! $this->app->get(SessionManager::class)->isStarted()) { | |||
There was a problem hiding this comment.
why do we need to modify this part?
| "license": "MIT", | ||
| "keywords": [ | ||
| "php", | ||
| "hyperf", |
There was a problem hiding this comment.
this package is designed for Hypervel only
|
|
||
| declare(strict_types=1); | ||
|
|
||
| /** |
|
|
||
| // @see: https://docs.sentry.io/platforms/php/guides/laravel/configuration/options/#traces_sampler | ||
| // 'traces_sampler' => function (Sentry\Tracing\SamplingContext $context): float { | ||
| // if (str_contains($context->getTransactionContext()->getDescription(), '/health')) { |
There was a problem hiding this comment.
there's no health route in hypervel
| ], | ||
|
|
||
| 'ignore_transactions' => [ | ||
| 'GET /health', |
There was a problem hiding this comment.
there's no health route in hypervel
| ], | ||
|
|
||
| 'ignore_commands' => [ | ||
| 'crontab:run', |
| class CoroutineAspect extends AbstractAspect | ||
| { | ||
| public array $classes = [ | ||
| 'Hypervel\Coroutine\Coroutine::create', |
There was a problem hiding this comment.
this should be Hyperf\Coroutine\Coroutine::create here?
| public function handleScheduledTaskStarting(ScheduledTaskStarting $event): void | ||
| { | ||
| // There is nothing for us to track if it's a background task since it will be handled by a separate process | ||
| if ($event->task->runInBackground) { |
There was a problem hiding this comment.
tasks which are marked as runInBackground will be executed in coroutines.
|
|
||
| public function register(): void | ||
| { | ||
| $logger = $this->container->get(LoggerInterface::class); |
There was a problem hiding this comment.
is it a redundant logger here?
| }); | ||
| } | ||
|
|
||
| protected function getClientIp(): ?string |
| \Psr\Http\Message\ServerRequestInterface::class, | ||
| ]; | ||
|
|
||
| public function process(ProceedingJoinPoint $proceedingJoinPoint) |
| new Pool( | ||
| $builder->getOptions(), | ||
| $this->app, | ||
| $this->app->get(ConfigInterface::class)->get('pools.sentry', []) |
There was a problem hiding this comment.
the config should be located under sentry?
| return $builder->setTransport($transport); | ||
| }); | ||
|
|
||
| $this->app->define(ClientInterface::class, function () { |
There was a problem hiding this comment.
why it's define here, but bind in the following binding?
|
|
||
| private function makeBacktraceHelper(): BacktraceHelper | ||
| { | ||
| return $this->container()->make(BacktraceHelper::class); |
| 'send_default_pii' => env('SENTRY_SEND_DEFAULT_PII', false), | ||
|
|
||
| // Must instanceof Psr\Log\LoggerInterface | ||
| // 'logger' => Hyperf\Contract\StdoutLoggerInterface::class, |
| 'guzzle' => env('SENTRY_BREADCRUMBS_GUZZLE', true), | ||
| ], | ||
|
|
||
| 'integrations' => [ |
| } catch (Exception $e) { | ||
| // We can assume the session store is not available here so there is no session key to retrieve | ||
| // We capture a generic exception to avoid breaking the application because some code paths can | ||
| // result in an exception other than the expected `Illuminate\Contracts\Container\BindingResolutionException` |
This pull request introduces significant adds Sentry integration to the project. The main changes the Sentry as a dependency and service provider. Additionally, test configuration and package registration have been updated accordingly.