Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ A major release `N` implies the previous release `N-1` will no longer receive up
## Unreleased

- Fix django legacy url resolver regex substitution due to upstream CVE-2021-44420 fix #1272
- Record lost `sample_rate` events only if tracing is enabled

## 1.5.0

Expand Down
10 changes: 5 additions & 5 deletions sentry_sdk/tracing.py
Original file line number Diff line number Diff line change
Expand Up @@ -543,24 +543,24 @@ def finish(self, hub=None):
hub = hub or self.hub or sentry_sdk.Hub.current
client = hub.client

if client is None:
# We have no client and therefore nowhere to send this transaction.
return None
Comment thread
rhcarvalho marked this conversation as resolved.

# This is a de facto proxy for checking if sampled = False
if self._span_recorder is None:
logger.debug("Discarding transaction because sampled = False")

# This is not entirely accurate because discards here are not
# exclusively based on sample rate but also traces sampler, but
# we handle this the same here.
Comment thread
sl0thentr0py marked this conversation as resolved.
if client and client.transport:
if client.transport and has_tracing_enabled(client.options):
client.transport.record_lost_event(
"sample_rate", data_category="transaction"
)

return None

if client is None:
# We have no client and therefore nowhere to send this transaction.
return None

if not self.name:
logger.warning(
"Transaction has no name, falling back to `<unlabeled transaction>`."
Expand Down
2 changes: 1 addition & 1 deletion sentry_sdk/tracing_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ def has_tracing_enabled(options):
# type: (Dict[str, Any]) -> bool
"""
Returns True if either traces_sample_rate or traces_sampler is
non-zero/defined, False otherwise.
defined, False otherwise.
Comment thread
rhcarvalho marked this conversation as resolved.
"""

return bool(
Expand Down
58 changes: 58 additions & 0 deletions tests/tracing/test_sampling.py
Original file line number Diff line number Diff line change
Expand Up @@ -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, []),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 traces_sample_rate is None.

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 0.0-False-... case has a report (one needs to think/know what reports are for reporting "lost" events).

Maybe a clearer test would be one where there's always some lost event, and the expected output for the None-... is that only the transaction-related report should be skipped, but not one about errors. Is that covered somewhere else?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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.
If we have 20 config parameters for sentry.init each taking 2 values (boolean), all possible combinations of those are 2^20 =1048576 total cases. Even more if those are not booleans. If any of those values is continuous, theoretically you need an infinite number of tests to test every possible state of your program/api surface.
Given this theoretical state of affairs, as engineers, we need to pick and choose where we write tests and not all tests add value/are worth adding.

I will add this one anyway but I just wanted to clarify how I think about testing for the future.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a clearer test would be one where there's always some lost event, and the expected output for the None-... is that only the transaction-related report should be skipped,

this tbh is a fairly contrived case which involves sample_rate too so I don't believe this belongs to the scope that these tests are trying to tackle.

],
)
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
)
Comment thread
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