Replace raven with sentry-sdk#55
Conversation
The raven library is deprecated; the official Sentry Python SDK is sentry-sdk. This migrates all Sentry integration to use the modern module-level sentry_sdk API. - Replace try/import raven with try/import sentry_sdk in process.py, consumer.py, and testing.py - Replace AsyncSentryClient(dsn, ...) with sentry_sdk.init() - Replace sentry_client.captureException() with sentry_sdk.capture_exception() inside a push_scope() context for extra/tag data - Replace sentry_client.tags_context() with sentry_sdk.set_tag() - Replace raven breadcrumbs.ignore_logger() with LoggingIntegration configured to suppress automatic log capture - Replace raven.processors.SanitizePasswordsProcessor with send_default_pii=False - sentry_client on Process is now a bool flag (True if initialized) rather than a client instance; consumer.sentry_client property reflects this - Add sentry-sdk>=2 as an optional extra in pyproject.toml Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughAdds an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Controller
participant Process
participant Consumer
participant Sentry as "sentry_sdk (optional)"
participant Scheduler as "Task Scheduler"
Controller->>Process: dispatch message / start child
Process->>Consumer: invoke_consumer(message)
Consumer->>Consumer: process message
alt Consumer raises exception
Consumer->>Process: report exception/context
Process->>Sentry: new_scope() / scope.set_extra(...)
Process->>Sentry: capture_exception(exc_info)
Process->>Scheduler: _schedule(retry/return coroutine)
Scheduler-->>Process: keep Task in _tasks (prevent GC)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Now that sentry_client is a simple bool flag rather than a raven Client instance, exposing it as a public API on Consumer serves no purpose. Consumers needing direct Sentry access should import sentry_sdk. The set_sentry_context and unset_sentry_context methods now check sentry_sdk and self._process.sentry_client directly. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
- Fix controller.py: missed in raven→sentry_sdk migration; now uses sentry_sdk.init() and capture_exception() - Fix process.py: replace deprecated sentry_sdk.push_scope() (removed in 2.x) with sentry_sdk.new_scope() - Fix unset_sentry_context: use sentry_sdk.get_current_scope().remove_tag() instead of set_tag(tag, None) which serialized as the string "None" - Remove dead _get_exc_info() method from consumer.py; all call sites were updated to use sys.exc_info() directly in the async migration - Replace asyncio.ensure_future() with asyncio.create_task() (idiomatic since Python 3.7) and add _schedule() helper with a task-set to prevent fire-and-forget tasks from being GC'd mid-execution - Remove redundant self._tcp_writer = None before reassignment in statsd._tcp_on_closed() Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rejected/process.py (1)
1008-1010:⚠️ Potential issue | 🟡 Minor
releasenever gets populated with the current call order.
_run()callssetup_sentry()beforesetup(), butself.consumer_versionis only assigned later inget_consumer(). So this branch is effectively dead and the migrated Sentry client will never get the intended release value.Also applies to: 1169-1170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rejected/process.py` around lines 1008 - 1010, setup_sentry() is being called from _run() before setup() and get_consumer() assign self.consumer_version, so the Sentry client never receives the intended release; fix by ensuring the release value is populated before creating the client—either move the call to setup_sentry() until after get_consumer()/setup() has assigned self.consumer_version, or change setup_sentry(self._kwargs['config'], self.consumer_name) to accept an explicit release argument (e.g., config_release or consumer_version) and pass the populated value, or update the Sentry client after get_consumer() via Sentry SDK set_release; adjust the code paths that call setup_sentry() (including the other occurrence at the later branch) to use the populated release.
🧹 Nitpick comments (1)
pyproject.toml (1)
42-42: Cap the optional Sentry extra to the major you migrated against.
sentry-sdk>=2will also admit future 3.x+ releases. Since the rest of this file already upper-bounds major versions, adding<3here would keep the optional extra aligned with the repository’s dependency strategy and avoid a surprise break on the next major.Suggested change
-sentry = ["sentry-sdk>=2"] +sentry = ["sentry-sdk>=2,<3"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` at line 42, Update the optional extras entry for Sentry to cap the major version to the one you migrated to: change the dependency spec string sentry = ["sentry-sdk>=2"] to include an upper bound for major 3 (e.g., sentry = ["sentry-sdk>=2,<3"]) so the project follows the existing strategy of upper-bounding major versions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rejected/controller.py`:
- Around line 15-17: The controller's sentry initialization (the sentry_sdk.init
call in the controller module) currently does not override the default
LoggingIntegration and therefore re-enables automatic log capture; update the
sentry_sdk.init call to pass LoggingIntegration(level=None, event_level=None)
(the same override used in process.py:setup_sentry) so automatic log capture is
suppressed—import or reference
sentry_sdk.integrations.logging.LoggingIntegration if needed and add the
integration to the init call alongside the existing options.
In `@rejected/process.py`:
- Around line 1043-1047: The current use of sentry_sdk.new_scope() sets
scope.set_extra('env', dict(os.environ)) (and similar extras at the other
occurrence) which exposes all environment variables; instead, change these
locations to only attach a whitelist of safe environment keys (e.g., grab
os.environ.get for specific keys like "SERVICE_ENV", "APP_VERSION") or sanitize
the dict before adding, or register a global sanitizer in setup_sentry() (e.g.,
an EventScrubber with recursive=True or a before_send hook) to remove secrets;
update the scope.set_extra calls (the with sentry_sdk.new_scope() block and the
other block that calls scope.set_extra at lines referenced) to use the
safe/filtered payload or rely on the sanitizer so no raw os.environ is sent to
Sentry.
---
Outside diff comments:
In `@rejected/process.py`:
- Around line 1008-1010: setup_sentry() is being called from _run() before
setup() and get_consumer() assign self.consumer_version, so the Sentry client
never receives the intended release; fix by ensuring the release value is
populated before creating the client—either move the call to setup_sentry()
until after get_consumer()/setup() has assigned self.consumer_version, or change
setup_sentry(self._kwargs['config'], self.consumer_name) to accept an explicit
release argument (e.g., config_release or consumer_version) and pass the
populated value, or update the Sentry client after get_consumer() via Sentry SDK
set_release; adjust the code paths that call setup_sentry() (including the other
occurrence at the later branch) to use the populated release.
---
Nitpick comments:
In `@pyproject.toml`:
- Line 42: Update the optional extras entry for Sentry to cap the major version
to the one you migrated to: change the dependency spec string sentry =
["sentry-sdk>=2"] to include an upper bound for major 3 (e.g., sentry =
["sentry-sdk>=2,<3"]) so the project follows the existing strategy of
upper-bounding major versions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e6810904-1c81-4389-8478-cb992d5498e7
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
pyproject.tomlrejected/consumer.pyrejected/controller.pyrejected/process.pyrejected/statsd.pyrejected/testing.py
💤 Files with no reviewable changes (1)
- rejected/statsd.py
Bugs fixed: - consumer.py: Consumer.returned property read self._message.redelivered instead of self._message.returned — silent behavioral bug since 3.17 - consumer.py: ChannelClosed/ConnectionClosed in execute() returned None, which on_processed() did not handle — message was silently counted as processed without ack; now returns MESSAGE_REQUEUE - consumer.py: drop_invalid_messages=False and error_max_retry=0 were silently ignored due to falsy-or chain; fixed with explicit None check - process.py: missing self.name argument in on_blocked/on_unblocked log format strings produced garbled log output - mcp.py: typo process vs processes in OSError cleanup handler caused dead processes to never be removed from the consumer dict, preventing replacement spawning Legacy patterns removed: - consumer.py: Python 2 unicode shim and its usage (unicode = str) - process.py: deprecated ssl.PROTOCOL_TLS (removed in Python 3.12); SSLContext() now uses the platform default - process.py: influxdb URL built with .format() replaced with f-string Performance: - consumer.py, process.py, data.py: replace max(start_time, time.time()) monotonicity guard with time.monotonic(), which never goes backward Quality: - consumer.py: bare Exception in require_setting -> ValueError; fix grammatical error "in to use" -> "to use" - consumer.py: bare string literal section divider replaced with comment Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
The blocking socket.sendall() was called from within the asyncio event loop on every message (via on_processed -> submit_statsd_measurements), stalling message processing if the statsd endpoint was slow. After the one-time blocking connect(), set the socket to non-blocking mode and use send() instead of sendall(). Statsd payloads are tiny (< 100 bytes), so a single send() completes in one syscall on any reasonable LAN connection. BlockingIOError on a full send buffer is caught by the existing OSError handler and triggers the failure callback. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Bounded pending deque (#2): Initialized with maxlen=qos_prefetch so memory usage is bounded by the configured prefetch count. RabbitMQ's QoS guarantee means the deque should never actually reach capacity in normal operation; the maxlen is a defensive backstop. Fix STATES class variable mutation (#3): Connection and Process were calling self.STATES[key] = value in __init__, which mutated the shared State base-class dict in-place — affecting all subclass instances. Each class now defines its own STATES as a class-level ClassVar that spreads the parent dict and adds its override, so each has an independent copy and __init__ no longer writes to shared state. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rejected/process.py (1)
370-390:⚠️ Potential issue | 🟠 MajorAddress the deprecated
ssl.SSLContext()usage and honor the documentedssl_options['protocol']configuration.The docstring (lines 373-381) documents
protocolas a valid configuration option, but line 390 creates the context without passing it. Whilessl.SSLContext()without a protocol argument doesn't raise aTypeError, modern Python versions issue aDeprecationWarningfor this pattern. Additionally, this represents a behavioral regression—users expecting the documentedprotocoloption to be honored will find it's now silently ignored.The suggested fix extracts and uses the protocol value:
Suggested fix
- context = ssl.SSLContext() + protocol = ssl_options.get('protocol', ssl.PROTOCOL_TLS_CLIENT) + if isinstance(protocol, str): + protocol = getattr(ssl, protocol) + context = ssl.SSLContext(protocol)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rejected/process.py` around lines 370 - 390, The _ssl_options method currently creates an SSLContext with ssl.SSLContext() which ignores the documented ssl_options['protocol'] and triggers a deprecation warning; change it to read protocol = ssl_options.get('protocol'), resolve it to a proper ssl module constant if it's a string (e.g. getattr(ssl, protocol) or accept an int directly), default to ssl.PROTOCOL_TLS_CLIENT (or another sensible default) when not provided, and pass that value into ssl.SSLContext(protocol) when constructing context so the protocol config is honored and the deprecation warning is avoided.
♻️ Duplicate comments (1)
rejected/process.py (1)
1046-1051:⚠️ Potential issue | 🟠 MajorStill avoid attaching raw environment variables to Sentry extras.
Line 1048 is still forwarding
dict(os.environ)to Sentry.send_default_pii
does not scrub custom extras, so secrets in environment variables can leak
unless you explicitly sanitize them.Safer direction
+ safe_env = { + key: os.environ[key] + for key in ('ENVIRONMENT', 'SERVICE') + if key in os.environ + } with sentry_sdk.new_scope() as scope: scope.set_extra('consumer_name', self.consumer_name) - scope.set_extra('env', dict(os.environ)) + scope.set_extra('env', safe_env)For sentry-sdk Python 2.x, does `send_default_pii=False` sanitize custom `scope.set_extra(...)` payloads like `dict(os.environ)`, or do I need an `EventScrubber`/`before_send` hook for that?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rejected/process.py` around lines 1046 - 1051, The code is adding raw environment variables to Sentry extras via sentry_sdk.new_scope and scope.set_extra('env', dict(os.environ)), which can leak secrets; replace this by sanitizing/filtering os.environ before setting it (e.g., whitelist allowed keys or remove keys matching common secret patterns like TOKEN, KEY, SECRET, PASSWORD, AWS, GCP, DB, etc.), or omit the env entirely and instead set only non-sensitive values (or a hashed/boolean indicator) and then call sentry_sdk.capture_exception with the sanitized scope; update sentry_sdk.new_scope / scope.set_extra usage to use a sanitize_env(env) helper and ensure sentry_sdk.capture_exception(exc_info, scope=scope) receives only the cleaned data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rejected/consumer.py`:
- Around line 440-445: The code sets Sentry tags using sentry_sdk.set_tag(...)
on the isolation scope but later removes them from the wrong scope; update the
removal to use the same isolation scope: where you currently call
sentry_sdk.get_current_scope().remove_tag(tag), replace it with
sentry_sdk.get_isolation_scope().remove_tag(tag) and ensure the same guard used
when setting tags (check sentry_sdk and self._process and
self._process.sentry_client) is applied before removing tags so tags are cleared
from the correct scope.
---
Outside diff comments:
In `@rejected/process.py`:
- Around line 370-390: The _ssl_options method currently creates an SSLContext
with ssl.SSLContext() which ignores the documented ssl_options['protocol'] and
triggers a deprecation warning; change it to read protocol =
ssl_options.get('protocol'), resolve it to a proper ssl module constant if it's
a string (e.g. getattr(ssl, protocol) or accept an int directly), default to
ssl.PROTOCOL_TLS_CLIENT (or another sensible default) when not provided, and
pass that value into ssl.SSLContext(protocol) when constructing context so the
protocol config is honored and the deprecation warning is avoided.
---
Duplicate comments:
In `@rejected/process.py`:
- Around line 1046-1051: The code is adding raw environment variables to Sentry
extras via sentry_sdk.new_scope and scope.set_extra('env', dict(os.environ)),
which can leak secrets; replace this by sanitizing/filtering os.environ before
setting it (e.g., whitelist allowed keys or remove keys matching common secret
patterns like TOKEN, KEY, SECRET, PASSWORD, AWS, GCP, DB, etc.), or omit the env
entirely and instead set only non-sensitive values (or a hashed/boolean
indicator) and then call sentry_sdk.capture_exception with the sanitized scope;
update sentry_sdk.new_scope / scope.set_extra usage to use a sanitize_env(env)
helper and ensure sentry_sdk.capture_exception(exc_info, scope=scope) receives
only the cleaned data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee0b6341-0e1b-4099-a9bd-d134e2e80d34
📒 Files selected for processing (5)
rejected/consumer.pyrejected/data.pyrejected/mcp.pyrejected/process.pyrejected/statsd.py
🚧 Files skipped from review as they are similar to previous changes (1)
- rejected/statsd.py
- controller.py: import LoggingIntegration and pass LoggingIntegration(level=None, event_level=None) to sentry_sdk.init so the controller suppresses automatic log capture consistently with the worker process - consumer.py: use get_isolation_scope() instead of get_current_scope() when removing Sentry tags; set_tag() writes to the isolation scope in sentry-sdk 2.x so tags must be removed from the same scope to actually clear between messages - process.py: filter os.environ before attaching it as a Sentry extra; keys containing KEY, SECRET, TOKEN, PASSWORD, DSN, CREDENTIAL, AUTH, or PRIVATE are excluded so database credentials and API keys are not forwarded to Sentry unfiltered Co-Authored-By: Gavin M. Roy <gavinmroy@gmail.com>
- process.py: honor the documented ssl_options['protocol'] config by reading it from the dict and passing it to ssl.SSLContext(); fall back to ssl.PROTOCOL_TLS_CLIENT when not provided to avoid the deprecation warning for a no-arg ssl.SSLContext() call - process.py: move setup_sentry() to after setup() / get_consumer() so that self.consumer_version is populated before sentry_sdk.init() is called; previously the release kwarg was always None and the intended version was never sent to Sentry - pyproject.toml: cap sentry-sdk optional extra to <3 to stay aligned with the repository's strategy of upper-bounding major versions Co-Authored-By: Gavin M. Roy <gavinmroy@gmail.com>
|
🤖 This comment was posted by Claude on behalf of @gmr PR Monitor SummaryIssues AddressedInline review threads (all resolved):
Outside-diff issues also fixed:
All 225 tests pass locally. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
rejected/process.py (1)
1049-1054:⚠️ Potential issue | 🔴 Critical
send_default_pii=Falsedoes not make the raw env extra safe.Sentry documents
send_default_piias controlling default PII collection and separately recommends SDK-side scrubbing for sensitive context. Because this code still attachesdict(os.environ)manually, credentials or tokens can be forwarded on every exception unless you whitelist or scrub that payload first. (docs.sentry.io)Also applies to: 1158-1160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rejected/process.py` around lines 1049 - 1054, The code is attaching the entire process environment via dict(os.environ) to Sentry scope, which can leak secrets despite send_default_pii=False; replace this with a scrubbed or whitelisted environment before calling scope.set_extra. Update the block that calls scope.set_extra('env', dict(os.environ)) (and the duplicate at the other location) to build a safe map (e.g., include only an explicit whitelist of non-sensitive keys or redact values for keys matching common secret patterns like PASS, TOKEN, KEY, SECRET) and attach that sanitized_env instead of the raw os.environ; ensure you reference the same symbols (scope.set_extra, self.consumer_name, sentry_sdk.capture_exception) so reviewers can find the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rejected/process.py`:
- Line 490: The pending queue uses a bounded
collections.deque(maxlen=self.qos_prefetch) which silently drops Message objects
when full; change the initialization of self.pending in the class (where pending
is created) to an unbounded deque (remove the maxlen argument) so messages are
never auto-discarded; keep using the same deque instance and flow (honoring
qos_prefetch elsewhere for consumption/flow control) and ensure any tests or
logic that assumed automatic dropping are updated to handle explicit ack/nack or
backpressure instead.
- Line 392: The current instantiation context = ssl.SSLContext() omits the
configured protocol and triggers deprecation warnings; change it to use the
configured ssl_options['protocol'] when present (e.g., context =
ssl.SSLContext(ssl_options['protocol'])) and otherwise fall back to
ssl.create_default_context() (e.g., context = ssl.create_default_context()) so
the code honors the documented ssl_options['protocol'] and avoids deprecated
default behavior; update the code around the SSLContext creation (the context
variable and any surrounding logic that reads ssl_options) to implement this
conditional selection.
---
Duplicate comments:
In `@rejected/process.py`:
- Around line 1049-1054: The code is attaching the entire process environment
via dict(os.environ) to Sentry scope, which can leak secrets despite
send_default_pii=False; replace this with a scrubbed or whitelisted environment
before calling scope.set_extra. Update the block that calls
scope.set_extra('env', dict(os.environ)) (and the duplicate at the other
location) to build a safe map (e.g., include only an explicit whitelist of
non-sensitive keys or redact values for keys matching common secret patterns
like PASS, TOKEN, KEY, SECRET) and attach that sanitized_env instead of the raw
os.environ; ensure you reference the same symbols (scope.set_extra,
self.consumer_name, sentry_sdk.capture_exception) so reviewers can find the
changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Co-Authored-By: Gavin M. Roy <gavinmroy@gmail.com>
collections.deque(maxlen=self.qos_prefetch) silently evicts messages from the left when it reaches capacity. With qos_prefetch=0 (unlimited) it creates a zero-capacity deque that drops every queued message. Even at the default of 1 it is an unnecessary footgun. The deque is only appended to when a message arrives while another is being processed, and RabbitMQ's qos_prefetch setting already limits how many unacked messages are in flight, so no upper bound on the deque is needed. Co-Authored-By: Gavin M. Roy <gavinmroy@gmail.com>
Summary
Migrate the Sentry integration from the deprecated
ravenlibrary to the officialsentry-sdk.Problem
ravenis the old Sentry Python SDK and has been deprecated in favor ofsentry-sdk. It depends ontornado(viaAsyncSentryClient), which was removed in the previous PR. The existing integration would silently no-op sinceravencan no longer be imported successfully in the modernized environment.Solution
raven/AsyncSentryClientimports withsentry_sdk/LoggingIntegrationinprocess.py,consumer.py, andtesting.pyAsyncSentryClient(dsn, ...)withsentry_sdk.init()insetup_sentry()sentry_client.captureException()withsentry_sdk.capture_exception()inside apush_scope()context for extra datasentry_client.tags_context()withsentry_sdk.set_tag()inset_sentry_context()raven.processors.SanitizePasswordsProcessorwithsend_default_pii=Falsebreadcrumbs.ignore_logger()withLoggingIntegration(level=None, event_level=None)to suppress automatic log captureProcess.sentry_clientis now a bool flag (Truewhen initialized) rather than a client instancesentry-sdk>=2as an optional extra (pip install rejected[sentry])Impact
Breaking change:
consumer.sentry_clientpreviously returned araven.Clientinstance that consumers could call directly (e.g.captureMessage). It now returnsTrue/None. Consumers needing direct Sentry access should importsentry_sdkdirectly.The
sentry_dsnconfig key is unchanged — no configuration migration needed.All 225 tests pass.
Summary by CodeRabbit
Chores
Bug Fixes