Skip to content
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

Warn when transaction entered without calling start_transaction #3003

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

szokeasaurusrex
Copy link
Member

This change should prevent confusion like what led to #2990 being opened.

Copy link
Member

@antonpirker antonpirker left a comment

Choose a reason for hiding this comment

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

Looks good, just one question.

sentry_sdk/tracing.py Outdated Show resolved Hide resolved
sentry_sdk/tracing.py Outdated Show resolved Hide resolved
sentry_sdk/tracing.py Show resolved Hide resolved
Comment on lines +417 to +422
The transaction will not be sent to Sentry. To fix, start the transaction by
passing it to sentry_sdk.start_transaction, like so:

with sentry_sdk.start_transaction(transaction):
# code here
"""
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The transaction will not be sent to Sentry. To fix, start the transaction by
passing it to sentry_sdk.start_transaction, like so:
with sentry_sdk.start_transaction(transaction):
# code here
"""
The transaction will not be sent to Sentry. To fix, start the transaction by
passing it to sentry_sdk.start_transaction, like so:
with sentry_sdk.start_transaction(transaction):
# code here
"""

Copy link
Member Author

Choose a reason for hiding this comment

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

This spacing is intentional; it makes the message appear nicely in the logs, like so:

 [sentry] WARNING: Transaction was entered without being started with sentry_sdk.start_transaction.
                   The transaction will not be sent to Sentry. To fix, start the transaction by
                   passing it to sentry_sdk.start_transaction, like so:

                   with sentry_sdk.start_transaction(transaction):
                       # code here            

@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/transaction-not-started-warning branch from 00c2bc7 to 5cd3884 Compare April 23, 2024 09:17
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/transaction-sampled-debug-message branch from a090a5c to f8586eb Compare April 25, 2024 12:58
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/transaction-not-started-warning branch from 5cd3884 to f1346b0 Compare April 25, 2024 13:02
`sampled` being set to `False` is not the only reason why the `_span_recorder` might be `None`. Another explanation is that the transaction was not started via `start_transaction`.
Transactions that are discarded because `_span_recorder` is `None` can also be discarded because they were not started with `sentry_sdk.start_transaction`. This change updates the debug message accordingly.

Fixes GH-3000
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/transaction-sampled-debug-message branch from f8586eb to d0fc544 Compare April 25, 2024 13:11
szokeasaurusrex and others added 3 commits April 25, 2024 15:12
…ransaction`

Users who enter a transaction without calling `start_transaction` likely intended to start the transaction, since without a call to `start_transaction`, their transaction will not get sent to Sentry. This warning message clarifies this behavior, and could help avoid the confusion that led to issue #2990.
The previous name, `_possibly_stated`, contains a typo, which this change fixes.

Co-authored-by: Anton Pirker <anton.pirker@sentry.io>
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/transaction-not-started-warning branch from f1346b0 to 84ab96a Compare April 25, 2024 13:12
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/transaction-sampled-debug-message branch from d0fc544 to 534a301 Compare April 25, 2024 13:26
Base automatically changed from szokeasaurusrex/transaction-sampled-debug-message to master April 25, 2024 13:40
Comment on lines +713 to +721
logger.warning(
"""Transaction was entered without being started with sentry_sdk.start_transaction.
The transaction will not be sent to Sentry. To fix, start the transaction by
passing it to sentry_sdk.start_transaction, like so:

with sentry_sdk.start_transaction(transaction):
# code here
"""
)
Copy link
Member

Choose a reason for hiding this comment

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

Log messages should not be multi-line because it makes it hard or impossible to grep through the logs (or do some other line based processing)

Please change the log message to be one line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants