feat(tracing): Record lost spans in client reports #3244

szokeasaurusrex · 2024-07-03T16:06:52Z

Resolves #3229
Resolves #3248

Note to reviewers: I have attempted to split the commits up into logical units, so you may wish to review the changes commit-by-commit.

szokeasaurusrex · 2024-07-03T16:08:31Z

Marked as draft, still need to test this

cleptric · 2024-07-03T16:48:36Z

We need to keep track of the amount of spans prior before_send_transaction and any event processors and calculate the delta afterwards, so we can report eventually dropped spans as well.

codecov · 2024-07-03T22:13:19Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.38%. Comparing base (ee84c81) to head (f33be4a).

Additional details and impacted files

@@                                       Coverage Diff                                       @@
##           szokeasaurusrex/use-capture_record_lost_event_calls-fixture    #3244      +/-   ##
===============================================================================================
+ Coverage                                                        79.35%   79.38%   +0.03%     
===============================================================================================
  Files                                                              132      132              
  Lines                                                            14227    14243      +16     
  Branches                                                          2987     2991       +4     
===============================================================================================
+ Hits                                                             11290    11307      +17     
+ Misses                                                            2092     2091       -1     
  Partials                                                           845      845

Files	Coverage Δ
sentry_sdk/client.py	`79.85% <100.00%> (+0.53%)`	⬆️
sentry_sdk/tracing.py	`73.60% <100.00%> (+0.05%)`	⬆️
sentry_sdk/transport.py	`81.31% <100.00%> (+0.58%)`	⬆️

szokeasaurusrex · 2024-07-04T08:04:54Z

We need to keep track of the amount of spans prior before_send_transaction and any event processors and calculate the delta afterwards, so we can report eventually dropped spans as well.

@cleptric, does this mean that we also need to report dropped spans in the before_send_transaction even if the transaction gets sent to Sentry?

For example, in the situation below, do we report any dropped spans to Sentry?

And for clarity, in the following situation where the transaction gets dropped after before_send/before_send_transaction, e.g. in the Transport, how many spans do we report as dropped, 4 or 6?

cleptric · 2024-07-04T09:24:34Z

The first example reports two dropped spans, the second example reports six dropped spans and one dropped transaction.

szokeasaurusrex · 2024-07-05T12:12:27Z

sentry_sdk/tracing.py

@@ -856,7 +854,8 @@ def finish(self, hub=None, end_timestamp=None):
                else:
                    reason = "sample_rate"

-                client.transport.record_lost_event(reason, data_category="transaction")
+                # No spans are discarded here because they were never recorded in the first place
+                client.transport.record_lost_transaction(reason, span_count=0)


Just realized I still need to test this codepath

`capture_record_lost_event_calls` replaces the `capture_client_reports` fixture. The fixture records calls to `Transport.record_lost_event` by noting the arguments passed to each call. This change is being introduced in preparation for #3244, which changes `Transport.record_lost_event`'s signature and behavior.

Make the `report["discarded_events"]` assertion logic (in `test_data_category_limits_reporting`) not rely on the ordering of events or any sorting logic. Done in preparation of #3244, where the sorting logic cannot be relied on anymore, since the same number of spans will be discarded as transactions.

…calls` Replace custom `record_lost_event` call capturing logic in `test_sampling.py` with the `capture_record_lost_event_calls` Pytest fixture. This change will simplify implementation of #3244.

Also, update existing transport tests so they pass against the changes introduced in this commit. Resolves #3229

- Add test for `record_lost_event` method's new `quantity` parameter - Add test for `record_lost_event` when passed a transaction item

antonpirker

Very nice!

`capture_record_lost_event_calls` replaces the `capture_client_reports` fixture. The fixture records calls to `Transport.record_lost_event` by noting the arguments passed to each call. This change is being introduced in preparation for getsentry#3244, which changes `Transport.record_lost_event`'s signature and behavior.

Make the `report["discarded_events"]` assertion logic (in `test_data_category_limits_reporting`) not rely on the ordering of events or any sorting logic. Done in preparation of getsentry#3244, where the sorting logic cannot be relied on anymore, since the same number of spans will be discarded as transactions.

…calls` Replace custom `record_lost_event` call capturing logic in `test_sampling.py` with the `capture_record_lost_event_calls` Pytest fixture. This change will simplify implementation of getsentry#3244.

szokeasaurusrex requested review from sentrivana and cleptric July 3, 2024 16:06

szokeasaurusrex changed the base branch from master to szokeasaurusrex/improve-transport-data-category-typing July 3, 2024 16:07

szokeasaurusrex changed the title ~~szokeasuaursrex/lost spans~~ feat(tracing): Record lost spans in client reports Jul 3, 2024

szokeasaurusrex marked this pull request as draft July 3, 2024 16:08

szokeasaurusrex linked an issue Jul 4, 2024 that may be closed by this pull request

Report dropped spans in client reports #3229

Closed

szokeasaurusrex force-pushed the szokeasaurusrex/improve-transport-data-category-typing branch 2 times, most recently from f098e6a to 854b46c Compare July 4, 2024 11:40

szokeasaurusrex force-pushed the szokeasuaursrex/lost-spans branch from 1cc0144 to 0ce0523 Compare July 4, 2024 11:41

szokeasaurusrex linked an issue Jul 4, 2024 that may be closed by this pull request

Improve record_lost_event parameter documentation #3248

Closed

szokeasaurusrex force-pushed the szokeasuaursrex/lost-spans branch 2 times, most recently from cc4291c to 3c62d91 Compare July 5, 2024 12:06

szokeasaurusrex marked this pull request as ready for review July 5, 2024 12:07

szokeasaurusrex commented Jul 5, 2024

View reviewed changes

szokeasaurusrex force-pushed the szokeasuaursrex/lost-spans branch from 3c62d91 to 7a142ef Compare July 8, 2024 08:52

szokeasaurusrex force-pushed the szokeasaurusrex/improve-transport-data-category-typing branch from 854b46c to 9b6a718 Compare July 8, 2024 08:52

Base automatically changed from szokeasaurusrex/improve-transport-data-category-typing to master July 8, 2024 11:59

szokeasaurusrex mentioned this pull request Jul 8, 2024

test: Introduce capture_record_lost_event_calls fixture #3254

Merged

szokeasaurusrex marked this pull request as draft July 8, 2024 14:16

szokeasaurusrex force-pushed the szokeasuaursrex/lost-spans branch from 7a142ef to 4c8dfff Compare July 8, 2024 15:02

szokeasaurusrex mentioned this pull request Jul 8, 2024

test(transport): Non-order-dependent discarded events assertion #3255

Merged

szokeasaurusrex force-pushed the szokeasuaursrex/lost-spans branch from 4c8dfff to fb4822a Compare July 8, 2024 15:47

szokeasaurusrex mentioned this pull request Jul 9, 2024

test(sampling): Replace custom logic with capture_record_lost_event_calls #3257

Merged

szokeasaurusrex force-pushed the szokeasuaursrex/lost-spans branch from 07d9da7 to f33be4a Compare July 9, 2024 09:09

szokeasaurusrex changed the base branch from master to szokeasaurusrex/use-capture_record_lost_event_calls-fixture July 9, 2024 09:09

szokeasaurusrex force-pushed the szokeasuaursrex/lost-spans branch 2 times, most recently from b98f150 to f8d866f Compare July 9, 2024 10:34

Base automatically changed from szokeasaurusrex/use-capture_record_lost_event_calls-fixture to master July 9, 2024 10:41

szokeasaurusrex marked this pull request as ready for review July 9, 2024 10:41

szokeasaurusrex requested a review from antonpirker July 9, 2024 10:41

szokeasaurusrex added 3 commits July 9, 2024 12:43

feat(tracing): Record lost spans in client reports

f84413d

Also, update existing transport tests so they pass against the changes introduced in this commit. Resolves #3229

test(transport): Test new client report features

c34a71e

- Add test for `record_lost_event` method's new `quantity` parameter - Add test for `record_lost_event` when passed a transaction item

test(client): Add tests for dropped span client reports

79e8970

szokeasaurusrex force-pushed the szokeasuaursrex/lost-spans branch from f8d866f to 79e8970 Compare July 9, 2024 10:43

antonpirker approved these changes Jul 9, 2024

View reviewed changes

szokeasaurusrex merged commit 79e8970 into master Jul 9, 2024
122 checks passed

szokeasaurusrex deleted the szokeasuaursrex/lost-spans branch July 9, 2024 14:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(tracing): Record lost spans in client reports #3244

feat(tracing): Record lost spans in client reports #3244

szokeasaurusrex commented Jul 3, 2024 •

edited

Loading

szokeasaurusrex commented Jul 3, 2024

cleptric commented Jul 3, 2024

codecov bot commented Jul 3, 2024 •

edited

Loading

szokeasaurusrex commented Jul 4, 2024

cleptric commented Jul 4, 2024

szokeasaurusrex Jul 5, 2024

antonpirker left a comment

feat(tracing): Record lost spans in client reports #3244

feat(tracing): Record lost spans in client reports #3244

Conversation

szokeasaurusrex commented Jul 3, 2024 • edited Loading

szokeasaurusrex commented Jul 3, 2024

cleptric commented Jul 3, 2024

codecov bot commented Jul 3, 2024 • edited Loading

Codecov Report

szokeasaurusrex commented Jul 4, 2024

cleptric commented Jul 4, 2024

szokeasaurusrex Jul 5, 2024

Choose a reason for hiding this comment

antonpirker left a comment

Choose a reason for hiding this comment

szokeasaurusrex commented Jul 3, 2024 •

edited

Loading

codecov bot commented Jul 3, 2024 •

edited

Loading