Repository navigation
feat(sentry)!: leave release-health sessions off and start log breadcrumbs at WARNING - #360
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #310. Closes #340.
This PR changes two Sentry defaults that cost time on every request or log record:
sentry_auto_session_trackingTrueFalsesentry_logging_breadcrumb_levellogging.INFOlogging.WARNING_warn_breadcrumb_level_ignoredused to compare againstlogging.INFOto tell whether the user had changed the field. It now compares againstlogging.WARNING, so an explicitINFOis treated as a change from the default. Whensentry_integrationsbrings its ownLoggingIntegration, orsentry_default_integrations=Falseis set, that explicitINFOis ignored and lite-bootstrap warns.Tests
Written first, all failing on main:
test_sentry_does_not_track_sessions_by_default: the client'sauto_session_trackingoption isFalse.test_sentry_breadcrumbs_start_at_warning_by_default: the breadcrumb handler's level isWARNING.test_sentry_error_event_carries_no_info_breadcrumbs_by_default: an INFO and a WARNING record logged before an error leave only the WARNING breadcrumb on the event. The test uses its own logger and filters by category, because the event also carries a subprocess breadcrumb from sentry-sdk working out the release.test_sentry_warns_when_a_non_default_info_breadcrumb_level_is_ignored: on main,INFOwas the default, so no warning was raised.test_sentry_logging_breadcrumb_level_controls_the_breadcrumb_handlergains aWARNINGcase. The full suite passes at 100% coverage on sentry-sdk 2.73.0 and 3.0.0b2, andtypasses against both.just lint,just adr-check,just docs-buildand lychee pass.Measurement
#340 asks for a rerun showing that an INFO call at
WARNINGcosts about what it costs atNone. Each mode ran in its own process: one structloglog.infocall throughFreeBootstrapper, output to/dev/null, median of 5 × 20000 calls, measured in-process on an M2 Mac.INFOWARNINGNoneThe new default saves about 5 µs per INFO record on both majors. I didn't rerun the session cost for #310. The 7.7 µs per request in the docs and the 7.2 to 8.7 µs re-measured in #310 both come from the issue, and the default change affects nothing else.
Docs
configuration.md: the two bullets give the new defaults and say how to get the old behaviour back. The paragraph saying breadcrumbs "stay on by default" is rewritten.performance.md:benchmarks/README.md:performance.mdincludes, dropssentry_auto_session_tracking=Falsebecause that is now the default.sentry_scenarios.py: theerrors_onlycomment says it reproduces 1.x's defaults. Its kwargs are unchanged, so the numbers measured with it are still valid.Breaking changes
sentry_auto_session_tracking=Trueto keep them. Only the HTTP frameworks opened sessions per request, so FastStream and Free behave the same as before.sentry_logging_breadcrumb_level=logging.INFOto keep them.