fix: strip skip_sentry from Sentry context; drop dead is_X_installed conjuncts#92
Merged
Merged
Conversation
…conjuncts DES-4: enrich_sentry_event_from_structlog_log was already returning None (suppressing the event) when skip_sentry was truthy, but for falsy values (False, missing, "") the flag itself was not stripped from the structlog payload before it was attached to event["contexts"]["structlog"]. Add "skip_sentry" to IGNORED_STRUCTLOG_ATTRIBUTES so the field never leaks into Sentry context noise. Regression test added as a parametrize case on the existing test_modify. DES-5: each affected instrument's is_ready() returned `<config check> and import_checker.is_X_installed`. The conjunct is provably dead: BaseBootstrapper calls check_dependencies() in _register_or_skip before instantiating the instrument, and only invokes is_ready() if check_dependencies() returned True. Drop the redundant conjunct from SentryInstrument, LoggingInstrument, OpenTelemetryInstrument, and PyroscopeInstrument. Behavior is unchanged. Closes DES-4 and DES-5 from the audit.
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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
1 participant
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.
Summary
Two small audit cleanups bundled:
skip_sentrywas already triggering event suppression when truthy, but for falsy values (False/missing/"") the flag itself wasn't stripped from the structlog payload and ended up as noise inevent["contexts"]["structlog"]. Add"skip_sentry"toIGNORED_STRUCTLOG_ATTRIBUTES. Regression added as a parametrize case on the existingtest_modify.is_ready()methods ended withand import_checker.is_X_installed. That conjunct is provably unreachable —BaseBootstrapper._register_or_skipcallscheck_dependencies()first and only invokesis_ready()if it returned True. Behavior is unchanged; cleanup makes the lifecycle easier to reason about.Closes DES-4 and DES-5 from an internal audit.
Test plan
just test -- tests/instruments/test_sentry_instrument.py -v— all parametrize cases pass.just test— full suite 84/84.just lint— clean (no F401 unused-import warnings;import_checkerstill used bycheck_dependencies()in every instrument).lite_bootstrap/bootstrappers/base.py:44-64—is_ready()is only invoked on an instrument that already passedcheck_dependencies().Notes for reviewer
import_checkerimport statement remains in all four instrument files (still referenced by eachcheck_dependencies()method).skip_sentry: trueparametrize case totest_modifyfor unit-level coverage of the suppression path (the integration testtest_sentry_instrument_with_structlog_erroralready covers it). Out of scope for this PR.🤖 Generated with Claude Code