-
-
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
feat(save_event): Always use cache for stacktrace processing #56413
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 |
---|---|---|
|
@@ -23,7 +23,7 @@ | |
from django.conf import settings | ||
from django.utils.encoding import force_str | ||
|
||
from sentry import eventtypes, features | ||
from sentry import eventtypes | ||
from sentry.db.models import NodeData | ||
from sentry.grouping.result import CalculatedHashes | ||
from sentry.interfaces.base import Interface, get_interfaces | ||
|
@@ -419,11 +419,7 @@ def normalize_stacktraces_for_grouping(self, grouping_config) -> None: | |
""" | ||
from sentry.stacktraces.processing import normalize_stacktraces_for_grouping | ||
|
||
normalize_stacktraces_for_grouping( | ||
self.data, | ||
grouping_config, | ||
load_stacktrace_from_cache=self.org_can_load_stacktrace_from_cache, | ||
) | ||
normalize_stacktraces_for_grouping(self.data, grouping_config) | ||
|
||
# We have modified event data, so any cached interfaces have to be reset: | ||
self.__dict__.pop("interfaces", None) | ||
|
@@ -491,10 +487,6 @@ def get_span_groupings( | |
def organization(self) -> Organization: | ||
return self.project.organization | ||
|
||
@property | ||
def org_can_load_stacktrace_from_cache(self) -> bool: | ||
return features.has("organizations:stacktrace-processing-caching", self.organization) | ||
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. This feature still shows up in |
||
|
||
@property | ||
def version(self) -> str: | ||
return cast(str, self.data.get("version", "5")) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -142,7 +142,6 @@ def apply_modifications_to_frame( | |
platform: str, | ||
exception_data: dict[str, Any], | ||
extra_fingerprint: str = "", | ||
load_stacktrace_from_cache: bool = False, | ||
) -> None: | ||
"""This applies the frame modifications to the frames itself. This does not affect grouping.""" | ||
in_memory_cache: dict[str, str] = {} | ||
|
@@ -158,12 +157,7 @@ def apply_modifications_to_frame( | |
cache_key = f"stacktrace_hash.{stacktrace_fingerprint}" | ||
use_cache = bool(stacktrace_fingerprint) | ||
if use_cache: | ||
frames_changed = _update_frames_from_cached_values( | ||
frames, | ||
cache_key, | ||
platform, | ||
load_from_cache=load_stacktrace_from_cache, | ||
) | ||
frames_changed = _update_frames_from_cached_values(frames, cache_key, platform) | ||
if frames_changed: | ||
logger.info("The frames have been loaded from the cache. Skipping some work.") | ||
return | ||
|
@@ -503,26 +497,21 @@ def visit_quoted_ident(self, node, children): | |
|
||
|
||
def _update_frames_from_cached_values( | ||
frames: Sequence[dict[str, Any]], cache_key: str, platform: str, load_from_cache: bool = False | ||
frames: Sequence[dict[str, Any]], cache_key: str, platform: str | ||
) -> bool: | ||
""" | ||
This will update the frames of the stacktrace if it's been cached. | ||
Set load_from_cache to True to actually change the frames. | ||
Returns if the merged has correctly happened. | ||
Returns True if the merged has correctly happened. | ||
""" | ||
frames_changed = False | ||
changed_frames_values = cache.get(cache_key, {}) | ||
|
||
# This helps tracking changes in the hit/miss ratio of the cache | ||
metrics.incr( | ||
f"{DATADOG_KEY}.cache.get", | ||
tags={ | ||
"success": bool(changed_frames_values), | ||
"platform": platform, | ||
"loading_from_cache": load_from_cache, | ||
}, | ||
tags={"success": bool(changed_frames_values), "platform": platform}, | ||
) | ||
if changed_frames_values and load_from_cache: | ||
if changed_frames_values: | ||
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. This is what enables it for everyone. |
||
try: | ||
for frame, changed_frame_values in zip(frames, changed_frames_values): | ||
if changed_frame_values.get("in_app") is not None: | ||
|
@@ -545,11 +534,7 @@ def _update_frames_from_cached_values( | |
|
||
metrics.incr( | ||
f"{DATADOG_KEY}.merged_cached_values", | ||
tags={ | ||
"success": frames_changed, | ||
"platform": platform, | ||
"loading_from_cache": load_from_cache, | ||
}, | ||
tags={"success": frames_changed, "platform": platform}, | ||
) | ||
return frames_changed | ||
|
||
|
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.
We remove the method in eventstore/models.