Fix statsd/prometheus/exception bugs and restrict metrics bind address#103
Conversation
Closes #77: statsd TCP mode no longer shuts the consumer down over a transient socket hiccup. The socket now uses a short blocking timeout instead of setblocking(False), _send() uses sendall() (avoiding partial writes), BlockingIOError/TimeoutError are logged and dropped rather than treated as fatal, and a hard OSError on TCP now routes through _tcp_on_closed() to reconnect (falling back to the failure callback only if reconnect fails) instead of leaving that path dead. Closes #93: STATSD_HOST/PORT/PREFIX env fallback now takes effect. The env resolution moved into StatsdConfig field default_factories so the documented precedence (explicit config > env var > hardcoded default) holds even though process.py builds the client from model_dump(), which always emits every field. The Client._setting() env lookup is left in place for direct/standalone use, keeping the module docstring accurate. Closes #94: RejectedException no longer discards value= or crashes on kwargs-only construction. raw_value defaults to '' (was a stray format template), __str__ formats value only when set and guards IndexError/KeyError, and __repr__ keys off value for consistency. Closes #95: custom Prometheus metrics are cached by their sanitized name (matching the registered name), so keys that sanitize alike (e.g. db.query / db-query) hit the cache instead of re-registering and raising Duplicated timeseries. observe() also wraps its body in try/except so a metrics error is logged rather than escaping into MCP stats collection. Closes #79: PrometheusConfig gains an address field defaulting to 127.0.0.1, and start() passes addr=address to start_http_server. This changes the bind default from the implicit 0.0.0.0 to loopback (the secure default the issue requests). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
📝 WalkthroughWalkthroughThis PR updates exception string behavior, adds Prometheus bind-address support and metric-key sanitization, hardens Prometheus observation error handling, and changes StatsD TCP send/reconnect behavior plus environment-driven configuration defaults. Tests were added for the new behavior. ChangesBehavior fixes bundle
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant MasterControlProgram
participant prometheus.start
participant prometheus_client
MasterControlProgram->>prometheus.start: start(port, address)
prometheus.start->>prometheus_client: start_http_server(port, addr=address)
sequenceDiagram
participant statsd.Client
participant TCP Socket
participant failure_callback
statsd.Client->>TCP Socket: sendall(data)
alt BlockingIOError or TimeoutError
TCP Socket-->>statsd.Client: raise
statsd.Client->>statsd.Client: log warning
else OSError
TCP Socket-->>statsd.Client: raise
statsd.Client->>statsd.Client: reconnect if TCP
statsd.Client->>failure_callback: call when previously connected
end
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rejected/exceptions.py (1)
26-34: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSingle positional-arg case leaves the value string in
self.args, corrupting.format()output.For
len(args) == 1,self.argsis always set toargsregardless of whether'value' not in kwargs, butraw_valuestill consumesargs[0]as the template. This means the value string ends up being passed to its own.format(*self.args, ...)call as a positional argument.For example,
MessageException('bad value: {0}')(a single positional message that happens to contain a placeholder) produces'bad value: bad value: {0}'instead of raisingIndexError/being returned as-is, or'bad value: {0}'unmodified. Compare with thelen(args) > 1branch, which correctly stripsargs[0]viaargs[1:]when'value' not in kwargs— the same stripping should apply whenlen(args) == 1.None of the added tests exercise this path (
test_plain_messageuses a template with no placeholders, so the bug is masked).🐛 Proposed fix
def __init__(self, *args: typing.Any, **kwargs: typing.Any) -> None: - if len(args) > 1: - self.args = args[1:] if 'value' not in kwargs else args - else: - self.args = args + if args and 'value' not in kwargs: + self.args = args[1:] + else: + self.args = args self.metric: str | None = kwargs.pop('metric', None) raw_value = kwargs.pop('value', args[0] if args else '')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rejected/exceptions.py` around lines 26 - 34, The single positional-arg path in the exception initializer leaves the template string in `self.args`, which then gets fed back into `.format()` and corrupts output. Update `MessageException.__init__` in `rejected/exceptions.py` so the `len(args) == 1` case handles `self.args` the same way as the `len(args) > 1` branch by stripping the message/template argument when `value` is not provided. Keep `raw_value` as the template source, but ensure the stored formatting args only contain real substitution values, and add coverage for a single-argument message containing placeholders.tests/test_exceptions.py (1)
1-49: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd a case for single positional arg +
.format()placeholder to catch theself.argsbug in__init__.None of these tests cover
MessageException('template with {0}')(single positional arg containing a format placeholder, novalue=kwarg, no extra args) — this is the exact scenario affected by theself.argsslicing bug flagged inrejected/exceptions.py. Adding it here would have caught the regression.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_exceptions.py` around lines 1 - 49, Add a regression test in RejectedExceptionTestCase for MessageException with a single positional format string containing a placeholder and no extra args or value= kwarg. The current tests miss the exact __init__ path in rejected.exceptions.MessageException where self.args slicing breaks formatting for this case. Verify str() returns the formatted message for MessageException('template with {0}') so the bug is caught if it regresses again.
🧹 Nitpick comments (2)
rejected/prometheus.py (2)
128-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog message omits the bind address.
The startup log only reports the port, not the address it's binding to, which is the actual security-relevant detail this PR is introducing (issue
#79).♻️ Proposed fix
- LOGGER.info('Prometheus metrics server started on port %d', port) + LOGGER.info( + 'Prometheus metrics server started on %s:%d', address, port + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rejected/prometheus.py` at line 128, The Prometheus startup log in prometheus.py only reports the port and omits the bind address, which is the key detail introduced by this change. Update the LOGGER.info call in the Prometheus server startup path to include the actual bind address alongside the port, using the relevant startup variables in the same block so the message clearly states where the server is listening.
224-247: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSingle try/except wraps unrelated metrics; one failure silently drops all others.
Wrapping the entire body in one
trymeans a failure in an early operation (e.g.duration_hist.observe) preventsage_hist, allcustom_durations,custom_counters, andcustom_gaugesfrom being recorded that call — not just the failing metric. This creates silent, broader-than-necessary data loss beyond what's needed to prevent a duplicate-timeseries error from propagating.Consider scoping the try/except per-metric-group (or at least per-key inside the loops) so an error in one custom metric doesn't suppress unrelated observations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rejected/prometheus.py` around lines 224 - 247, The current Prometheus observation flow in the metrics recording function uses one broad try/except around all metric updates, so a failure in one call can prevent unrelated metrics from being recorded. Refactor the logic in the observation routine to scope exception handling more narrowly around each metric group, such as the duration histogram, message_age histogram, and each custom metric loop in separate try/except blocks, so one bad metric does not suppress the others while still handling duplicate-timeseries errors gracefully.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rejected/models.py`:
- Around line 53-55: The STATSD_PORT parsing in the config field initializer is
brittle because invalid env values can crash Config.model_validate. Update the
port field in rejected/models.py by guarding the os.environ lookup used in the
default_factory so it falls back to 8125 when parsing fails, and keep the fix
localized to the same field definition that currently calls
int(os.environ.get(...)).
In `@rejected/statsd.py`:
- Around line 164-166: The TCP timeout is being reused for both connection
establishment and socket writes in the StatsD client, which can make slow
remotes fail too early during connect. Update the connection path in the statsd
socket handling logic so the connect attempt uses a longer bounded timeout, then
switch to the short TCP timeout only after a successful connection for
send/write operations; use the relevant StatsD send/connect method and the
sock.settimeout flow to locate the change.
- Around line 118-126: The send path in statsd.py currently treats TimeoutError
from _tcp_writer.sendall() the same as a dropped metric, but a timeout after
partial writes leaves the TCP stream unusable for the next metric. Update the
write logic in the send method to distinguish TimeoutError from BlockingIOError,
and when sendall() times out call _tcp_on_closed() so the TCP writer is reset
before the next write. Keep the existing LOGGER.warning behavior for the
non-fatal case and reference the _tcp_writer, _tcp_on_closed(), and sendall()
paths when making the change.
In `@tests/test_prometheus.py`:
- Around line 122-127: The explicit-address test in test_explicit_address still
contains the same S104 literal-address issue, so add the matching Ruff
suppression in this test to make the assertion and mocked start_http_server call
pass a clean lint run. Apply the same targeted nosec/ignore-style suppression
used elsewhere in the test suite around the prometheus.start invocation and the
addr assertion so both 0.0.0.0 occurrences are covered without changing the test
behavior.
- Around line 21-23: The test in test_address_configurable is tripping Ruff S104
because it hardcodes the literal “0.0.0.0” in a string assertion. Update the
test to avoid embedding that exact literal directly—use a shared test constant
or construct the expected address dynamically—and keep the round-trip check
against PrometheusConfig so the behavior is still verified without triggering
the static analysis rule.
---
Outside diff comments:
In `@rejected/exceptions.py`:
- Around line 26-34: The single positional-arg path in the exception initializer
leaves the template string in `self.args`, which then gets fed back into
`.format()` and corrupts output. Update `MessageException.__init__` in
`rejected/exceptions.py` so the `len(args) == 1` case handles `self.args` the
same way as the `len(args) > 1` branch by stripping the message/template
argument when `value` is not provided. Keep `raw_value` as the template source,
but ensure the stored formatting args only contain real substitution values, and
add coverage for a single-argument message containing placeholders.
In `@tests/test_exceptions.py`:
- Around line 1-49: Add a regression test in RejectedExceptionTestCase for
MessageException with a single positional format string containing a placeholder
and no extra args or value= kwarg. The current tests miss the exact __init__
path in rejected.exceptions.MessageException where self.args slicing breaks
formatting for this case. Verify str() returns the formatted message for
MessageException('template with {0}') so the bug is caught if it regresses
again.
---
Nitpick comments:
In `@rejected/prometheus.py`:
- Line 128: The Prometheus startup log in prometheus.py only reports the port
and omits the bind address, which is the key detail introduced by this change.
Update the LOGGER.info call in the Prometheus server startup path to include the
actual bind address alongside the port, using the relevant startup variables in
the same block so the message clearly states where the server is listening.
- Around line 224-247: The current Prometheus observation flow in the metrics
recording function uses one broad try/except around all metric updates, so a
failure in one call can prevent unrelated metrics from being recorded. Refactor
the logic in the observation routine to scope exception handling more narrowly
around each metric group, such as the duration histogram, message_age histogram,
and each custom metric loop in separate try/except blocks, so one bad metric
does not suppress the others while still handling duplicate-timeseries errors
gracefully.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 04e46672-2d47-4742-8dc8-7b233ebead01
📒 Files selected for processing (8)
rejected/exceptions.pyrejected/mcp.pyrejected/models.pyrejected/prometheus.pyrejected/statsd.pytests/test_exceptions.pytests/test_prometheus.pytests/test_statsd.py
- exceptions: fix RejectedException.__init__ so a lone positional template is not fed back into its own .format() call; strip the message arg whenever no value= kwarg is given (covers len(args) == 1) - statsd: reconnect on TimeoutError from sendall() since a partial write leaves the TCP stream unusable for the next metric - statsd: use a longer bounded connect timeout, applying the short TCP_TIMEOUT only after the connection is established - models: guard STATSD_PORT parsing so a non-numeric env var falls back to 8125 instead of aborting Config.model_validate - prometheus: include the bind address in the startup log message - tests: add regression coverage for the above Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the CodeRabbit review in b4b9b5e. Notes on the out-of-diff findings and nitpicks: Fixed
Declined
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rejected/statsd.py (1)
118-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
BlockingIOErrorbranch looks unreachable given current socket config.Both sockets in this class are now configured so this branch should never fire: the TCP socket uses
settimeout(TCP_CONNECT_TIMEOUT)thensettimeout(TCP_TIMEOUT)(both positive floats), and the UDP socket sets no timeout at all (fully blocking). Per Python socket semantics,BlockingIOErroris only raised when a socket's timeout is exactly0(non-blocking mode viasetblocking(False)/settimeout(0)); a positive timeout raisessocket.timeout/TimeoutErrorinstead, which is already handled by the block right below it (lines 128-133).The previous
setblocking(False)call for the TCP socket was removed as part of this diff (per the summarized diff), so there's no remaining code path that could triggerBlockingIOErrorhere — it's effectively dead defensive code that may mislead future maintainers into thinking backpressure is split across two branches when only theTimeoutErrorpath is actually exercised.Worth confirming intent: if this is purely defensive/future-proofing (e.g., anticipating a future non-blocking mode), consider a short comment noting that; otherwise it can be removed to avoid confusion in this hot networking path.
🧹 Possible cleanup if confirmed unreachable
- except BlockingIOError as error: - LOGGER.warning( - 'Dropping statsd metric, socket not ready: %s', error - ) - except TimeoutError as error: + except TimeoutError as error: LOGGER.warning('Timeout sending statsd metric: %s', error) if self._tcp_writer: # sendall may have written a partial metric, leaving the # stream unusable; reconnect before the next write. self._tcp_on_closed()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rejected/statsd.py` around lines 118 - 133, The BlockingIOError handling in statsd.py appears unreachable with the current socket setup in the Statsd client, since _tcp_writer uses positive timeouts and _udp_sock is blocking, so only TimeoutError should occur. Review the send path in the Statsd class (including the sendall/sendto try block and the _tcp_on_closed reconnect logic) and either remove the dead BlockingIOError branch or add a brief comment if you intend to keep it for future non-blocking mode support.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@rejected/statsd.py`:
- Around line 118-133: The BlockingIOError handling in statsd.py appears
unreachable with the current socket setup in the Statsd client, since
_tcp_writer uses positive timeouts and _udp_sock is blocking, so only
TimeoutError should occur. Review the send path in the Statsd class (including
the sendall/sendto try block and the _tcp_on_closed reconnect logic) and either
remove the dead BlockingIOError branch or add a brief comment if you intend to
keep it for future non-blocking mode support.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 85b22d97-f830-4fd3-9510-445dc86074b3
📒 Files selected for processing (6)
rejected/exceptions.pyrejected/models.pyrejected/prometheus.pyrejected/statsd.pytests/test_exceptions.pytests/test_statsd.py
🚧 Files skipped from review as they are similar to previous changes (5)
- rejected/models.py
- tests/test_exceptions.py
- rejected/prometheus.py
- tests/test_statsd.py
- rejected/exceptions.py
Both sockets are configured with blocking semantics: the TCP socket uses positive settimeout() values and the UDP socket has no timeout, so sendall()/sendto() raise TimeoutError rather than BlockingIOError (the latter only fires on non-blocking sockets). The branch was dead code left over from the setblocking(False) approach this PR replaced. Dropped it and the test that simulated the impossible case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the remaining nitpick from the latest review ( Confirmed unreachable: the TCP socket uses positive |
1 participant
Summary
Fixes 5 verified bugs in
statsd.py,prometheus.py,models.py, andexceptions.py.Problem / Solution
send()BlockingIOErrortriggered the failure callback (Process.stop), shutting the consumer down over a metrics buffer hiccup; partial writes corrupted lines; the reconnect path was never wired → switched to a shortsettimeout()+sendall(),BlockingIOError/timeouts are droppable, and_tcp_on_closed()reconnect is now wired in.STATSD_HOST/PORT/PREFIXenv fallback could never fire becausemodel_dump()always populated every field → env resolution moved intoStatsdConfigfield defaults (precedence: explicit config > env var > default), working through the production path with noprocess.pychange.RejectedExceptiondiscarded the keywordvalueand raisedIndexErrorin__str__for kwargs-only construction →raw_valuedefaults to'';__str__formats only when set and guards format errors.ValueErrorand broke MCP stats collection → cache keyed by sanitized name;observe()body guarded so it can't propagate into stats collection.0.0.0.0with no way to restrict → addedPrometheusConfig.address(default127.0.0.1, a secure-by-default change) and wired it through themcp.pyprometheus.start(...)call site.Testing
Full suite green (235 tests) + ruff clean. New
tests/test_exceptions.py,tests/test_prometheus.py, and new statsd test cases. (test_prometheus.pyskips if theprometheus_clientextra isn't installed.)Closes #77
Closes #79
Closes #93
Closes #94
Closes #95
Summary by CodeRabbit
127.0.0.1).STATSD_HOST,STATSD_PORT, andSTATSD_PREFIX, withSTATSD_PORTsafely falling back to8125when invalid.str()/repr()formatting fallbacks when templates/arguments don’t match.