Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/Client.php
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,10 @@ public function captureException(\Throwable $exception, ?Scope $scope = null, ?E
*/
public function captureEvent(Event $event, ?EventHint $hint = null, ?Scope $scope = null): ?EventId
{
$event = $this->prepareEvent($event, $hint, $scope);
// Client reports don't need to be augmented in the prepareEvent pipeline.
if ($event->getType() !== EventType::clientReport()) {
$event = $this->prepareEvent($event, $hint, $scope);
}
Comment on lines +181 to +184

Choose a reason for hiding this comment

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

m: From the comment that I left above when using captureEvent, I would consider just sending the reports immediately to the transport instead of complicating the logic here. We should also verify that we don't rate limit the reports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have plans to change the way events are processed and sent, right now it all goes through the same captureEvent workflow but we will split it up into separate pipelines. I would keep this as is for now and do the refactoring later

Copy link

Choose a reason for hiding this comment

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

Client reports route through full captureEvent pipeline unnecessarily

Low Severity

Client reports are sent by calling $client->captureEvent() from ClientReportAggregator::flush(), which forces an event-type check into captureEvent to skip prepareEvent. This adds branching complexity to a core method and couples internal telemetry to the user-facing event capture path. The LogsAggregator and MetricsAggregator use $hub->captureEvent(), making this approach inconsistent. Sending directly through the transport (as the reviewer suggested) would avoid complicating captureEvent and naturally bypass rate limiting, scope enrichment, and lastEventId concerns.

Additional Locations (1)

Fix in Cursor Fix in Web

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is good feedback and we will restructure the flow of different telemetry and non telemetry events in the future so we can get independent e.g. captureClientReports or captureLogs pipelines which will only enrich and augment data that is necessary


if ($event === null) {
return null;
Expand Down
84 changes: 84 additions & 0 deletions src/ClientReport/ClientReportAggregator.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
<?php

declare(strict_types=1);

namespace Sentry\ClientReport;

use Sentry\Event;
use Sentry\State\HubAdapter;
use Sentry\Transport\DataCategory;

class ClientReportAggregator
{
/**
* @var self
*/
private static $instance;

/**
* Nested array for local aggregation. The first key is the category and the second one is the reason.
*
* ```
* [
* 'example-category' => [
* 'example-reason' => 10
* ]
* ]
*```
*
* @var array<array<string, int>>
*/
private $reports = [];

public function add(DataCategory $category, Reason $reason, int $quantity): void
{
$category = $category->getValue();
$reason = $reason->getValue();
if ($quantity <= 0) {
$client = HubAdapter::getInstance()->getClient();
if ($client !== null) {
$logger = $client->getOptions()->getLoggerOrNullLogger();
$logger->debug('Dropping Client report with category={category} and reason={reason} because quantity is zero or negative ({quantity})', [
'category' => $category,
'reason' => $reason,
'quantity' => $quantity,
]);
}

return;
}
$this->reports[$category][$reason] = ($this->reports[$category][$reason] ?? 0) + $quantity;
}

public function flush(): void
{
if (empty($this->reports)) {
return;
}
$reports = [];
foreach ($this->reports as $category => $reasons) {
foreach ($reasons as $reason => $quantity) {
$reports[] = new DiscardedEvent($category, $reason, $quantity);
}
}
$event = Event::createClientReport();
$event->setClientReports($reports);

$client = HubAdapter::getInstance()->getClient();

// Reset the client reports only if we successfully sent an event. If it fails it
// can be sent on the next flush, or it gets discarded anyway.
if ($client !== null && $client->captureEvent($event) !== null) {
$this->reports = [];
}
}

public static function getInstance(): self
{
if (self::$instance === null) {
self::$instance = new self();
}

return self::$instance;
}
}
45 changes: 45 additions & 0 deletions src/ClientReport/DiscardedEvent.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
<?php

declare(strict_types=1);

namespace Sentry\ClientReport;

class DiscardedEvent
{
/**
* @var string
*/
private $reason;

/**
* @var string
*/
private $category;

/**
* @var int
*/
private $quantity;

public function __construct(string $category, string $reason, int $quantity)
{
$this->category = $category;
$this->reason = $reason;
$this->quantity = $quantity;
}

public function getCategory(): string
{
return $this->category;
}

public function getQuantity(): int
{
return $this->quantity;
}

public function getReason(): string
{
return $this->reason;
}
}
102 changes: 102 additions & 0 deletions src/ClientReport/Reason.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
<?php

declare(strict_types=1);

namespace Sentry\ClientReport;

class Reason
{
/**
* @var string
*/
private $value;

/**
* @var array<self>
*/
private static $instances = [];

public function __construct(string $value)
{
$this->value = $value;
}

public static function queueOverflow(): self
{
return self::getInstance('queue_overflow');
}

public static function cacheOverflow(): self
{
return self::getInstance('cache_overflow');
}

public static function bufferOverflow(): self
{
return self::getInstance('buffer_overflow');
}

public static function ratelimitBackoff(): self
{
return self::getInstance('ratelimit_backoff');
}

public static function networkError(): self
{
return self::getInstance('network_error');
}

public static function sampleRate(): self
{
return self::getInstance('sample_rate');
}

public static function beforeSend(): self
{
return self::getInstance('before_send');
}

public static function eventProcessor(): self
{
return self::getInstance('event_processor');
}

public static function sendError(): self
{
return self::getInstance('send_error');
}

public static function internalSdkError(): self
{
return self::getInstance('internal_sdk_error');
}

public static function insufficientData(): self
{
return self::getInstance('insufficient_data');
}

public static function backpressure(): self
{
return self::getInstance('backpressure');
}

public function getValue(): string
{
return $this->value;
}

public function __toString()
{
return $this->value;
}

private static function getInstance(string $value): self
{
if (!isset(self::$instances[$value])) {
self::$instances[$value] = new self($value);
}

return self::$instances[$value];
}
}
29 changes: 29 additions & 0 deletions src/Event.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

namespace Sentry;

use Sentry\ClientReport\DiscardedEvent;
use Sentry\Context\OsContext;
use Sentry\Context\RuntimeContext;
use Sentry\Logs\Log;
Expand Down Expand Up @@ -210,6 +211,11 @@ final class Event
*/
private $profile;

/**
* @var DiscardedEvent[]
*/
private $clientReports = [];

private function __construct(?EventId $eventId, EventType $eventType)
{
$this->id = $eventId ?? EventId::generate();
Expand Down Expand Up @@ -252,6 +258,11 @@ public static function createMetrics(?EventId $eventId = null): self
return new self($eventId, EventType::metrics());
}

public static function createClientReport(?EventId $eventId = null): self
{
return new self($eventId, EventType::clientReport());
}

/**
* Gets the ID of this event.
*/
Expand Down Expand Up @@ -978,4 +989,22 @@ public function getTraceId(): ?string

return null;
}

/**
* @param DiscardedEvent[] $clientReports
*/
public function setClientReports(array $clientReports): self
{
$this->clientReports = $clientReports;

return $this;
}

/**
* @return DiscardedEvent[]
*/
public function getClientReports(): array
{
return $this->clientReports;
}
}
15 changes: 15 additions & 0 deletions src/EventType.php
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,11 @@ public static function metrics(): self
return self::getInstance('trace_metric');
}

public static function clientReport(): self
{
return self::getInstance('client_report');
}

/**
* List of all cases on the enum.
*
Expand All @@ -65,6 +70,7 @@ public static function cases(): array
self::checkIn(),
self::logs(),
self::metrics(),
self::clientReport(),
];
}

Expand All @@ -73,12 +79,21 @@ public function requiresEventId(): bool
switch ($this) {
case self::metrics():
case self::logs():
case self::clientReport():
return false;
default:
return true;
}
}

/**
* Returns false if rate limiting should not be applied.
*/
public function requiresRateLimiting(): bool
{
return $this !== self::clientReport();
}

public function __toString(): string
{
return $this->value;
Expand Down
31 changes: 31 additions & 0 deletions src/Serializer/EnvelopItems/ClientReportItem.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
<?php

declare(strict_types=1);

namespace Sentry\Serializer\EnvelopItems;

use Sentry\ClientReport\DiscardedEvent;
use Sentry\Event;
use Sentry\Util\JSON;

class ClientReportItem implements EnvelopeItemInterface
{
public static function toEnvelopeItem(Event $event): ?string
{
$reports = $event->getClientReports();

$headers = ['type' => 'client_report'];
$body = [
'timestamp' => $event->getTimestamp(),
'discarded_events' => array_map(static function (DiscardedEvent $report) {
return [
'category' => $report->getCategory(),
'reason' => $report->getReason(),
'quantity' => $report->getQuantity(),
];
}, $reports),
];

return \sprintf("%s\n%s", JSON::encode($headers), JSON::encode($body));
}
}
Loading