-
Notifications
You must be signed in to change notification settings - Fork 55
feat(prompts): add capture_errors option for error tracking #427
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
base: master
Are you sure you want to change the base?
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 |
|---|---|---|
|
|
@@ -509,6 +509,121 @@ def test_handle_variables_with_dots(self): | |
| self.assertEqual(result, "Company: Acme") | ||
|
|
||
|
|
||
| class TestPromptsCaptureErrors(TestPrompts): | ||
| """Tests for the capture_errors option.""" | ||
|
|
||
| @patch("posthog.ai.prompts._get_session") | ||
| def test_capture_exception_called_on_fetch_failure_with_fallback( | ||
|
Comment on lines
+512
to
+516
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. Tests are not parameterized The new Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! Prompt To Fix With AIThis is a comment left during a code review.
Path: posthog/test/ai/test_prompts.py
Line: 512:516
Comment:
**Tests are not parameterized**
The new `TestPromptsCaptureErrors` cases are all the same shape (setup fetch failure/success → call `get()` → assert on `capture_exception`). This increases repetition and makes it easier to miss scenarios when adding new branches. The repo preference is parameterized tests; these can be collapsed into a single parameterized test that covers (fallback vs stale cache vs re-raise vs success) and toggles (`capture_errors` on/off, client present/absent).
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise. |
||
| self, mock_get_session | ||
| ): | ||
| """Should call capture_exception on fetch failure when capture_errors=True.""" | ||
| mock_get = mock_get_session.return_value.get | ||
| mock_get.side_effect = Exception("Network error") | ||
|
|
||
| posthog = self.create_mock_posthog() | ||
| prompts = Prompts(posthog, capture_errors=True) | ||
|
|
||
| result = prompts.get("test-prompt", fallback="fallback prompt") | ||
|
|
||
| self.assertEqual(result, "fallback prompt") | ||
| posthog.capture_exception.assert_called_once() | ||
| captured_exc = posthog.capture_exception.call_args[0][0] | ||
| self.assertIn("Network error", str(captured_exc)) | ||
|
|
||
| @patch("posthog.ai.prompts._get_session") | ||
| @patch("posthog.ai.prompts.time.time") | ||
| def test_capture_exception_called_on_fetch_failure_with_stale_cache( | ||
| self, mock_time, mock_get_session | ||
| ): | ||
| """Should call capture_exception when falling back to stale cache.""" | ||
| mock_get = mock_get_session.return_value.get | ||
| mock_get.side_effect = [ | ||
| MockResponse(json_data=self.mock_prompt_response), | ||
| Exception("Network error"), | ||
| ] | ||
| mock_time.return_value = 1000.0 | ||
|
|
||
| posthog = self.create_mock_posthog() | ||
| prompts = Prompts(posthog, capture_errors=True) | ||
|
|
||
| # First call populates cache | ||
| prompts.get("test-prompt", cache_ttl_seconds=60) | ||
|
|
||
| # Expire cache | ||
| mock_time.return_value = 1061.0 | ||
|
|
||
| # Second call falls back to stale cache | ||
| result = prompts.get("test-prompt", cache_ttl_seconds=60) | ||
| self.assertEqual(result, self.mock_prompt_response["prompt"]) | ||
| posthog.capture_exception.assert_called_once() | ||
|
|
||
| @patch("posthog.ai.prompts._get_session") | ||
| def test_capture_exception_called_when_error_is_raised(self, mock_get_session): | ||
| """Should call capture_exception even when the error is re-raised (no fallback, no cache).""" | ||
| mock_get = mock_get_session.return_value.get | ||
| mock_get.side_effect = Exception("Network error") | ||
|
|
||
| posthog = self.create_mock_posthog() | ||
| prompts = Prompts(posthog, capture_errors=True) | ||
|
|
||
| with self.assertRaises(Exception): | ||
| prompts.get("test-prompt") | ||
|
|
||
| posthog.capture_exception.assert_called_once() | ||
|
|
||
| @patch("posthog.ai.prompts._get_session") | ||
| def test_no_capture_exception_when_capture_errors_is_false(self, mock_get_session): | ||
| """Should NOT call capture_exception when capture_errors=False (default).""" | ||
| mock_get = mock_get_session.return_value.get | ||
| mock_get.side_effect = Exception("Network error") | ||
|
|
||
| posthog = self.create_mock_posthog() | ||
| prompts = Prompts(posthog) | ||
|
|
||
| prompts.get("test-prompt", fallback="fallback prompt") | ||
|
|
||
| posthog.capture_exception.assert_not_called() | ||
|
|
||
| @patch("posthog.ai.prompts._get_session") | ||
| def test_no_capture_exception_without_client(self, mock_get_session): | ||
| """Should not error when capture_errors=True but no client provided.""" | ||
| mock_get = mock_get_session.return_value.get | ||
| mock_get.side_effect = Exception("Network error") | ||
|
|
||
| prompts = Prompts(personal_api_key="phx_test_key", capture_errors=True) | ||
|
|
||
| result = prompts.get("test-prompt", fallback="fallback prompt") | ||
|
|
||
| self.assertEqual(result, "fallback prompt") | ||
|
|
||
| @patch("posthog.ai.prompts._get_session") | ||
| def test_no_capture_exception_on_successful_fetch(self, mock_get_session): | ||
| """Should NOT call capture_exception on successful fetch.""" | ||
| mock_get = mock_get_session.return_value.get | ||
| mock_get.return_value = MockResponse(json_data=self.mock_prompt_response) | ||
|
|
||
| posthog = self.create_mock_posthog() | ||
| prompts = Prompts(posthog, capture_errors=True) | ||
|
|
||
| prompts.get("test-prompt") | ||
|
|
||
| posthog.capture_exception.assert_not_called() | ||
|
|
||
| @patch("posthog.ai.prompts._get_session") | ||
| def test_capture_exception_failure_does_not_affect_fallback(self, mock_get_session): | ||
| """If capture_exception itself throws, the fallback should still be returned.""" | ||
| mock_get = mock_get_session.return_value.get | ||
| mock_get.side_effect = Exception("Network error") | ||
|
|
||
| posthog = self.create_mock_posthog() | ||
| posthog.capture_exception.side_effect = Exception("capture failed") | ||
| prompts = Prompts(posthog, capture_errors=True) | ||
|
|
||
| result = prompts.get("test-prompt", fallback="fallback prompt") | ||
|
|
||
| self.assertEqual(result, "fallback prompt") | ||
|
|
||
|
|
||
| class TestPromptsClearCache(TestPrompts): | ||
| """Tests for the Prompts.clear_cache() method.""" | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| VERSION = "7.8.3" | ||
| VERSION = "7.8.4" | ||
|
|
||
| if __name__ == "__main__": | ||
| print(VERSION, end="") # noqa: T201 |
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.
Unchecked capture_exception call
_maybe_capture_error()assumes the providedposthogobject has acapture_exceptionmethod. Withcapture_errors=True, passing a PostHog client version (or a stub) that lackscapture_exceptionwill raiseAttributeError, which then gets swallowed by the broadexcept Exception, meaning errors silently won’t be reported (and it’s hard to diagnose). Consider guarding the call (e.g.,hasattr(self._client, "capture_exception")) and/or logging at least once at WARNING when the method is missing so users know why capture isn’t happening.Prompt To Fix With AI