-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
chore(similarity): Add logging for over 30 system frames #81130
Changes from all commits
236040f
b895b89
a8eb0cb
fd71e4d
9d84081
8f08f43
454fa0c
99e3700
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 | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -179,18 +179,22 @@ def get_stacktrace_string(data: dict[str, Any]) -> str: | |||||||
html_frame_count = 0 # for a temporary metric | ||||||||
stacktrace_str = "" | ||||||||
found_non_snipped_context_line = False | ||||||||
is_frames_truncated = False | ||||||||
|
||||||||
metrics.distribution("seer.grouping.exceptions.length", len(exceptions)) | ||||||||
|
||||||||
def _process_frames(frames: list[dict[str, Any]]) -> list[str]: | ||||||||
nonlocal frame_count | ||||||||
nonlocal html_frame_count | ||||||||
nonlocal found_non_snipped_context_line | ||||||||
nonlocal is_frames_truncated | ||||||||
frame_strings = [] | ||||||||
|
||||||||
contributing_frames = [ | ||||||||
frame for frame in frames if frame.get("id") == "frame" and frame.get("contributes") | ||||||||
] | ||||||||
if len(contributing_frames) + frame_count > MAX_FRAME_COUNT: | ||||||||
is_frames_truncated = True | ||||||||
Comment on lines
+196
to
+197
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. It might just be unclear variable naming (and not having read through the entire algorithm) which is getting me, but why are we adding contributing frames to all frames? Also, nit: Comparisons return a bool, so we can just use that directly.
Suggested change
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. we're doing this to see if the length of all the contributing frames we've accumulated so far (either from this current exception or some previous ones) is greater than 30 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. Okay, so basially 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. other way around, but yes that's the idea 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. Got it, thanks for the explanation! |
||||||||
contributing_frames = _discard_excess_frames( | ||||||||
contributing_frames, MAX_FRAME_COUNT, frame_count | ||||||||
) | ||||||||
|
@@ -287,6 +291,14 @@ def _process_frames(frames: list[dict[str, Any]]) -> list[str]: | |||||||
}, | ||||||||
) | ||||||||
|
||||||||
if is_frames_truncated and not app_hash: | ||||||||
logger_extra = { | ||||||||
"project_id": data.get("project_id", ""), | ||||||||
"event_id": data.get("event_id", ""), | ||||||||
"hash": system_hash, | ||||||||
} | ||||||||
logger.info("grouping.similarity.over_threshold_system_only_frames", extra=logger_extra) | ||||||||
|
||||||||
return stacktrace_str.strip() | ||||||||
|
||||||||
|
||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -5,9 +5,11 @@ | |||||||||||||||||||||||||||||||||||||||||
from sentry import options | ||||||||||||||||||||||||||||||||||||||||||
from sentry.conf.server import SEER_SIMILARITY_MODEL_VERSION | ||||||||||||||||||||||||||||||||||||||||||
from sentry.eventstore.models import Event | ||||||||||||||||||||||||||||||||||||||||||
from sentry.grouping.grouping_info import get_grouping_info_from_variants | ||||||||||||||||||||||||||||||||||||||||||
from sentry.grouping.ingest.seer import get_seer_similar_issues, should_call_seer_for_grouping | ||||||||||||||||||||||||||||||||||||||||||
from sentry.models.grouphash import GroupHash | ||||||||||||||||||||||||||||||||||||||||||
from sentry.seer.similarity.types import SeerSimilarIssueData | ||||||||||||||||||||||||||||||||||||||||||
from sentry.seer.similarity.utils import MAX_FRAME_COUNT | ||||||||||||||||||||||||||||||||||||||||||
from sentry.testutils.cases import TestCase | ||||||||||||||||||||||||||||||||||||||||||
from sentry.testutils.helpers.eventprocessing import save_new_event | ||||||||||||||||||||||||||||||||||||||||||
from sentry.testutils.helpers.options import override_options | ||||||||||||||||||||||||||||||||||||||||||
|
@@ -306,3 +308,47 @@ def test_returns_no_grouphash_and_empty_metadata_if_no_similar_group_found(self) | |||||||||||||||||||||||||||||||||||||||||
expected_metadata, | ||||||||||||||||||||||||||||||||||||||||||
None, | ||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
@patch("sentry.seer.similarity.utils.logger") | ||||||||||||||||||||||||||||||||||||||||||
def test_too_many_only_system_frames(self, mock_logger: Mock) -> None: | ||||||||||||||||||||||||||||||||||||||||||
type = "FailedToFetchError" | ||||||||||||||||||||||||||||||||||||||||||
value = "Charlie didn't bring the ball back" | ||||||||||||||||||||||||||||||||||||||||||
context_line = f"raise {type}('{value}')" | ||||||||||||||||||||||||||||||||||||||||||
new_event = Event( | ||||||||||||||||||||||||||||||||||||||||||
project_id=self.project.id, | ||||||||||||||||||||||||||||||||||||||||||
event_id="22312012112120120908201304152013", | ||||||||||||||||||||||||||||||||||||||||||
data={ | ||||||||||||||||||||||||||||||||||||||||||
"title": f"{type}('{value}')", | ||||||||||||||||||||||||||||||||||||||||||
"exception": { | ||||||||||||||||||||||||||||||||||||||||||
"values": [ | ||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||
"type": type, | ||||||||||||||||||||||||||||||||||||||||||
"value": value, | ||||||||||||||||||||||||||||||||||||||||||
"stacktrace": { | ||||||||||||||||||||||||||||||||||||||||||
"frames": [ | ||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||
"function": f"play_fetch_{i}", | ||||||||||||||||||||||||||||||||||||||||||
"filename": f"dogpark{i}.py", | ||||||||||||||||||||||||||||||||||||||||||
"context_line": context_line, | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
for i in range(MAX_FRAME_COUNT + 1) | ||||||||||||||||||||||||||||||||||||||||||
] | ||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
] | ||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||
"platform": "python", | ||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||
variants = new_event.get_grouping_variants() | ||||||||||||||||||||||||||||||||||||||||||
get_seer_similar_issues(new_event, variants) | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
grouping_info = get_grouping_info_from_variants(variants) | ||||||||||||||||||||||||||||||||||||||||||
mock_logger.info.assert_called_with( | ||||||||||||||||||||||||||||||||||||||||||
"grouping.similarity.over_threshold_system_only_frames", | ||||||||||||||||||||||||||||||||||||||||||
extra={ | ||||||||||||||||||||||||||||||||||||||||||
"project_id": "", | ||||||||||||||||||||||||||||||||||||||||||
"event_id": "", | ||||||||||||||||||||||||||||||||||||||||||
"hash": grouping_info["system"]["hash"], | ||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+346
to
+354
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. Nit:
Suggested change
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. the case we're testing here is calling this also doesn't work since grouping info is calculated in the function. we could mock it, but I think the other tests cover this. 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. Ah, I see. Okay, great. If we were doing this for real I might suggest moving adding the info into the grouping info helper itself (then you only have to do it in one place, I think?), but again, since this is temporary, I say 🚢 . |
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.
Nit:
or
or
etc.