-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix(aci): Quiet Redis exception noise #99955
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| import logging | ||
| from collections.abc import Generator | ||
| from contextlib import contextmanager | ||
| from typing import Any, Literal | ||
|
|
||
| import sentry_sdk | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| # sentry_sdk doesn't export these. | ||
| _Event = Any | ||
| _ExcInfo = Any | ||
|
|
||
| SentryLevel = Literal["error", "warning", "info"] | ||
|
|
||
|
|
||
| @contextmanager | ||
| def set_sentry_exception_levels( | ||
| exception_levels: dict[type[BaseException], SentryLevel], | ||
| ) -> Generator[None]: | ||
| """ | ||
| Context manager that sets up a Sentry error processor to set | ||
| specific exception types to configured Sentry levels. | ||
|
|
||
| Args: | ||
| exception_levels: Map of exception type to their desired Sentry levels | ||
| Note that type matching is done by equality, not isinstance. | ||
| """ | ||
|
|
||
| def process_error(event: _Event, exc_info: _ExcInfo) -> _Event | None: | ||
| exc = exc_info[1] | ||
| exc_type = type(exc) | ||
|
|
||
| # Check if this exception type should have its level overridden | ||
| if exc_type in exception_levels: | ||
| new_level = exception_levels[exc_type] | ||
| event["level"] = new_level | ||
|
|
||
| return event | ||
|
|
||
| with sentry_sdk.new_scope() as scope: | ||
| scope.add_error_processor(process_error) | ||
| yield | ||
|
|
||
|
|
||
| @contextmanager | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤔 - think it would be helpful for one of us to give a brown bag on this? (
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, it's an important tool in the toolbox. If it's not known, sharing that it exists (and maybe ContextVars?) would be helpful. |
||
| def quiet_redis_noise() -> Generator[None]: | ||
| """ | ||
| Context manager that sets up a Sentry error processor to quiet Redis noise. | ||
| Specifically, the current library versions report TimeoutError and MovedError | ||
| internally even when they're being appropriately handled, and it's incorrect for | ||
| those to be treated as errors in Sentry. | ||
| """ | ||
| from redis.exceptions import TimeoutError | ||
| from rediscluster.exceptions import MovedError # type: ignore[attr-defined] | ||
|
|
||
| with set_sentry_exception_levels({TimeoutError: "info", MovedError: "info"}): | ||
| yield | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| from collections.abc import Callable | ||
| from typing import Any | ||
| from unittest.mock import Mock, patch | ||
|
|
||
| from sentry.workflow_engine.utils.sentry_level_utils import set_sentry_exception_levels | ||
|
|
||
|
|
||
| class TestSetSentryExceptionLevels: | ||
| def test_basic_functionality_minimal_mocking(self) -> None: | ||
| with patch("sentry_sdk.new_scope") as mock_scope: | ||
| mock_scope_instance = Mock() | ||
| mock_scope.return_value.__enter__ = Mock(return_value=mock_scope_instance) | ||
| mock_scope.return_value.__exit__ = Mock(return_value=None) | ||
|
|
||
| # Use a single-element list to capture the processor | ||
| captured_processors: list[Callable[[Any, Any], Any]] = [] | ||
| mock_scope_instance.add_error_processor = captured_processors.append | ||
|
|
||
| # Use the context manager with exception type as key | ||
| with set_sentry_exception_levels({ValueError: "warning"}): | ||
| pass | ||
|
|
||
| # Basic validation that processor was captured | ||
| assert len(captured_processors) == 1 | ||
| processor = captured_processors[0] | ||
|
|
||
| # Test that it correctly processes a ValueError | ||
| event = {"level": "error", "other_data": "preserved"} | ||
| exc = ValueError("test error") | ||
| exc_info = (ValueError, exc, None) | ||
|
|
||
| result = processor(event, exc_info) | ||
|
|
||
| # Verify the level was changed and other data preserved | ||
| assert result["level"] == "warning" | ||
| assert result["other_data"] == "preserved" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mind adding a quick test file for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had one, and I deleted it because it was a lot of mocks talking to mocks. The Sentry SDK doesn't offer much for validating internal behavior, so I pivoted to just reproducing the reporting situation with a global 'before_send' hook that logs to prove to myself it would do as expected.
I'll add in a basic test though.