feat: add noop client implementation - #1365
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
4337b6c to
415d82c
Compare
415d82c to
18b7a4a
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
18b7a4a to
94121a8
Compare
59c6bdc to
095e3a7
Compare
095e3a7 to
7ecc977
Compare
| func (hub *Hub) CaptureCheckIn(checkIn *CheckIn, monitorConfig *MonitorConfig) *EventID { | ||
| client, scope := hub.Client(), hub.Scope() | ||
| if client == nil { | ||
| return nil |
There was a problem hiding this comment.
Check-in returns ID when disabled
Medium Severity
CaptureCheckIn no longer returns nil when no real client is bound. The hub-level client == nil guard was removed, but Client.CaptureCheckIn never checks IsEnabled() and still returns a check-in ID after CaptureEvent discards the event. Callers that treat a non-nil ID as a successful capture will think the check-in was recorded when the noop client dropped it.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 7ecc977. Configure here.
This PR changes the client boundaries so that we always return a non nil client.
7ecc977 to
8fc2ecf
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8fc2ecf. Configure here.
| // the utility methods like CaptureException. The return value is the | ||
| // event ID. In case Sentry is disabled or event was dropped, the return value will be nil. | ||
| func (client *Client) CaptureEvent(event *Event, hint *EventHint, scope EventModifier) *EventID { | ||
| if !client.IsEnabled() { |
There was a problem hiding this comment.
Check-in succeeds on disabled client
Medium Severity
CaptureCheckIn still builds a check-in and returns its ID after the hub nil-client guard was removed. CaptureEvent now drops the payload when the client is disabled, so callers get a non-nil ID even though nothing was captured. Cron flows that treat a returned ID as success will look healthy while monitors never receive the check-in.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8fc2ecf. Configure here.
| func (client *Client) Recover(err any, hint *EventHint, scope EventModifier) *EventID { | ||
| if !client.IsEnabled() { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Disabled Recover skips panic recovery
Medium Severity
Recover returns immediately when the client is disabled, before calling recover(). Direct use as a panic handler with a noop client therefore leaves the original panic unrecovered. RecoverWithContext does not have this early return, so the two APIs now behave inconsistently for disabled clients.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8fc2ecf. Configure here.
2 participants
Description
This adds the Client interface and noop and default client implementations for the SDK. This better aligns the behavior with the current client spec and removes the need to always nil check any client.
Issues
Changelog Entry Instructions
To add a custom changelog entry, uncomment the section above. Supports:
For more details: custom changelog entries
Reminders
feat:,fix:,ref:,meta:)