feat(core): Record callback_error client reports for throwing user callbacks - #23903
feat(core): Record callback_error client reports for throwing user callbacks#23903msonnb wants to merge 3 commits into
callback_error client reports for throwing user callbacks#23903Conversation
size-limit report 📦
|
dd74588 to
8f068cc
Compare
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 8f068cc. Configure here.
| samplingContext: SamplingContext, | ||
| sampleRand: number, | ||
| ): [sampled: boolean, sampleRate?: number, localSampleRateWasApplied?: boolean] { | ||
| ): SamplingDecision { |
There was a problem hiding this comment.
m: this is API breaking, since sampleSpan is exported. I agree that the object is easier to read though . IIRC, we used the named tuple here for bundle size minimization but this is less readable than the refactor.
Breaking here isn't a big deal though, so we can also do it if the tuple no longer works. But if we do, let's add a note in the migration guide.
There was a problem hiding this comment.
oops, missed the public exported. was just for readability so i reverted to the tuple
| () => processor({ ...event }, hint), | ||
| () => null, | ||
| () => { | ||
| throw CALLBACK_ERROR; |
There was a problem hiding this comment.
m: throwing here and in client.ts made me a bit suspicious because we have to be really careful to also catch our throws. Looks like there's a case in replay where we also call prepareEvent which doesn't try/catch the call. I think this only concerns event processor throws though. This leads to us trying to re-send the replay because we assume a network error. Ultimately, we stop recording.
I think we have two options how to avoid this:
- we try/catch in Replay so that this doesn't happen
- we don't throw at all. Instead, we could directly record an dropped event in
notifyEventProcessorsand distinguish there.
Tbqh: I haven't thought this through end-to-end, so maybe 1 is easier. I don't see us reusing prepareEvent much in the foreseeable future. But happy to let you make the call (also feel free to come up with something else).
There was a problem hiding this comment.
so I went with option 2 for now, which means that notifyEventProcessors (which is also publicly exported from core) gains an extra (but optional and thus backwards-compatible) onDrop callback since it doesn't have a reference to the client currently. I guess we could also just pass the client directly, which would introduce more coupling, but probably save a few bytes.
something entirely else would be introducing some kind of result type like { ok: true; event } | { ok: false; error } but that feels a bit overkill and would change the function signatures entirely.
side note: I feel like Java-like checked exceptions would really help here 😅
…callbacks Events, logs, metrics and root spans dropped because a user callback threw were reported with the same outcome as a legitimate filter (`before_send`, `event_processor`, `sample_rate`). A dedicated `callback_error` reason makes them distinguishable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
8f068cc to
9a3f5ed
Compare
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.
Reviewed by Cursor Bugbot for commit 9a3f5ed. Configure here.
Treat callback failures as drops at the callback boundary. Event processors and beforeSend hooks now keep their normal Event | null return shape and report callback_error through dedicated callbacks instead of throwing or returning CALLBACK_ERROR. prepareEvent records processor drops, while Client records beforeSend drops. This keeps the sentinel from escaping those pipelines and avoids duplicate client reports. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Co-Authored-By: OpenAI Codex <codex@openai.com>
9a3f5ed to
efbaddb
Compare
| // Taken from baseclient's `_processEvent` method, where this is handled for errors/transactions | ||
| client.recordDroppedEvent('event_processor', 'replay'); |
There was a problem hiding this comment.
l: might be worth adding a comment here that this is handled within prepareEvent now
|
thanks for making the changes! |
2 participants
Drops caused by a throwing user callback were reported with the same client report reason as a legitimate filter. They now use a new
callback_errorreason so the two are distinguishable.Part of #23755