feat(client-reports): Add support for client reports#1978
feat(client-reports): Add support for client reports#1978
Conversation
# Conflicts: # src/Serializer/PayloadSerializer.php
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| } | ||
|
|
||
| if ($envelopeHeader === null) { | ||
| return \sprintf("{}\n%s", implode("\n", array_filter($items))); |
There was a problem hiding this comment.
Client report envelopes omit standard headers
Medium Severity
PayloadSerializer::serialize() special-cases EventType::clientReport() by emitting an empty envelope header ({}) instead of the usual sent_at/dsn/sdk fields. If Sentry/Relay expects these envelope headers for routing or validation, client reports may be rejected even though other event types succeed.
There was a problem hiding this comment.
That's according to spec: https://develop.sentry.dev/sdk/telemetry/client-reports/#full-envelope
giortzisg
left a comment
giortzisg
left a comment
There was a problem hiding this comment.
LGTM overall, left some comments to verify that we don't rate-limit the reports
| ]; | ||
| $envelopeHeader = null; | ||
| if ($event->getType() !== EventType::clientReport()) { | ||
| // @see https://develop.sentry.dev/sdk/envelopes/#envelope-headers |
There was a problem hiding this comment.
m: The spec mentions that it's recommended to send sent_at headers always. Let's verify if we should send it or not here.
| // Client reports don't need to be augmented in the prepareEvent pipeline. | ||
| if ($event->getType() !== EventType::clientReport()) { | ||
| $event = $this->prepareEvent($event, $hint, $scope); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Client reports overwrite Hub's
lastEventIdtracking- Client report flush now sends via the bound client directly instead of Hub::captureEvent(), so flushing no longer overwrites the hub’s user-facing lastEventId.
Or push these changes by commenting:
@cursor push e7cd5c73a3
Preview (e7cd5c73a3)
diff --git a/src/ClientReport/ClientReportAggregator.php b/src/ClientReport/ClientReportAggregator.php
--- a/src/ClientReport/ClientReportAggregator.php
+++ b/src/ClientReport/ClientReportAggregator.php
@@ -64,9 +64,11 @@
$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 (HubAdapter::getInstance()->captureEvent($event) !== null) {
+ if ($client !== null && $client->captureEvent($event) !== null) {
$this->reports = [];
}
}
diff --git a/tests/ClientReport/ClientReportAggregatorTest.php b/tests/ClientReport/ClientReportAggregatorTest.php
--- a/tests/ClientReport/ClientReportAggregatorTest.php
+++ b/tests/ClientReport/ClientReportAggregatorTest.php
@@ -66,6 +66,20 @@
$this->assertSame(40, $report->getQuantity());
}
+ public function testFlushDoesNotOverwriteLastEventId(): void
+ {
+ $hub = SentrySdk::getCurrentHub();
+ $eventId = $hub->captureMessage('foo');
+
+ $this->assertNotNull($eventId);
+ $this->assertSame($eventId, $hub->getLastEventId());
+
+ ClientReportAggregator::getInstance()->add(DataCategory::profile(), Reason::eventProcessor(), 10);
+ ClientReportAggregator::getInstance()->flush();
+
+ $this->assertSame($eventId, $hub->getLastEventId());
+ }
+
public function testNegativeQuantityDiscarded(): void
{
ClientReportAggregator::getInstance()->add(DataCategory::profile(), Reason::eventProcessor(), -10);
Applied via @cursor push command
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| // Client reports don't need to be augmented in the prepareEvent pipeline. | ||
| if ($event->getType() !== EventType::clientReport()) { | ||
| $event = $this->prepareEvent($event, $hint, $scope); | ||
| } |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
3 participants
Adds support for ClientReports.
This PR only adds the possibility to send client reports, it does not collect them yet