-
Notifications
You must be signed in to change notification settings - Fork 643
fix(client-reports): Record lost sample_rate events only if tracing is enabled
#1268
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
c28d52a
ca84d57
6950af8
8cb3ef7
e69f164
b8f0f60
848f1ce
2e9ff10
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -284,3 +284,61 @@ def test_warns_and_sets_sampled_to_false_on_invalid_traces_sampler_return_value( | |
| transaction = start_transaction(name="dogpark") | ||
| logger.warning.assert_any_call(StringContaining("Given sample rate is invalid")) | ||
| assert transaction.sampled is False | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "traces_sample_rate,sampled_output,reports_output", | ||
| [ | ||
| (None, False, []), | ||
| (0.0, False, [("sample_rate", "transaction")]), | ||
| (1.0, True, []), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My curiosity, were there no tests covering this case before? The key behavior this PR is changing is the first case, when I read the test names again, and while it is better now that each input and output is an explicit parameter, it is not so obvious why only the Maybe a clearer test would be one where there's always some lost event, and the expected output for the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no that's not covered afaik, will add one. One high level comment I want to make here re: my personal testing philosophy is about the impossibility of testing all cases. I will add this one anyway but I just wanted to clarify how I think about testing for the future.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see your extrapolation. But we're not trying to test all combinations of input, but rather behaviors, and there are not so many interesting behaviors to test here. For testing arbitrary inputs then we can rely on other tools such as fuzzing, and not hand-written unit tests.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
this tbh is a fairly contrived case which involves |
||
| ], | ||
| ) | ||
| def test_records_lost_event_only_if_traces_sample_rate_enabled( | ||
| sentry_init, traces_sample_rate, sampled_output, reports_output, monkeypatch | ||
| ): | ||
| reports = [] | ||
|
|
||
| def record_lost_event(reason, data_category=None, item=None): | ||
| reports.append((reason, data_category)) | ||
|
|
||
| sentry_init(traces_sample_rate=traces_sample_rate) | ||
|
|
||
| monkeypatch.setattr( | ||
| Hub.current.client.transport, "record_lost_event", record_lost_event | ||
| ) | ||
|
sl0thentr0py marked this conversation as resolved.
|
||
|
|
||
| transaction = start_transaction(name="dogpark") | ||
| assert transaction.sampled is sampled_output | ||
| transaction.finish() | ||
|
|
||
| assert reports == reports_output | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "traces_sampler,sampled_output,reports_output", | ||
| [ | ||
| (None, False, []), | ||
| (lambda _x: 0.0, False, [("sample_rate", "transaction")]), | ||
| (lambda _x: 1.0, True, []), | ||
| ], | ||
| ) | ||
| def test_records_lost_event_only_if_traces_sampler_enabled( | ||
| sentry_init, traces_sampler, sampled_output, reports_output, monkeypatch | ||
| ): | ||
| reports = [] | ||
|
|
||
| def record_lost_event(reason, data_category=None, item=None): | ||
| reports.append((reason, data_category)) | ||
|
|
||
| sentry_init(traces_sampler=traces_sampler) | ||
|
|
||
| monkeypatch.setattr( | ||
| Hub.current.client.transport, "record_lost_event", record_lost_event | ||
| ) | ||
|
|
||
| transaction = start_transaction(name="dogpark") | ||
| assert transaction.sampled is sampled_output | ||
| transaction.finish() | ||
|
|
||
| assert reports == reports_output | ||
Uh oh!
There was an error while loading. Please reload this page.