-
-
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
ref(seer grouping): Use frame tallies for excess frame check #82414
Merged
lobsterkatie
merged 9 commits into
master
from
kmclb-use-tallies-for-excessive-frame-seer-filter
Dec 20, 2024
Merged
ref(seer grouping): Use frame tallies for excess frame check #82414
lobsterkatie
merged 9 commits into
master
from
kmclb-use-tallies-for-excessive-frame-seer-filter
Dec 20, 2024
+361
−44
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
Scope: Backend
Automatically applied to PRs that change backend components
label
Dec 19, 2024
❌ 3 Tests Failed:
View the top 3 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
lobsterkatie
force-pushed
the
kmclb-use-tallies-for-excessive-frame-seer-filter
branch
from
December 19, 2024 22:45
e363adc
to
bc2eb07
Compare
armenzg
approved these changes
Dec 20, 2024
lobsterkatie
deleted the
kmclb-use-tallies-for-excessive-frame-seer-filter
branch
December 20, 2024 17:44
andrewshie-sentry
pushed a commit
that referenced
this pull request
Jan 2, 2025
This is the first step in simplifying the way we apply the stacktrace length filter to events sent to Seer. It puts in place the new machinery, but does not yet remove the old machinery, which will happen in a follow-up PR. In the meantime, though, even though the old code is still there, the new code should supplant it, since its check comes before the existing one - meaning anything which should get filtered should be flagged by the new check and shouldn't make it through to the existing check. I used different names for the metrics so that we can make sure that's the case before we remove the existing code. Key changes: - A new `has_too_many_contributing_frames` to be used in all the places we send events to Seer (ingest, backfill, similar issues tab), which collects a `grouping.similarity.frame_count_filter` metric, regardless of (and tagged with) the referrer. - A new ingest-specific `_has_too_many_contributing_frames` wrapper for the above, which also records a `did_call_seer` metric. - In both the backfill and similar issues tab endpoint code, getting `grouping_info` is now split into two steps, first getting the variants and then getting grouping info from the variants, so that the variants can be fed to `has_too_many_contributing_frames`. - All `get_stacktrace_string` tests whose names included some variation of "too many fames" have been renamed, so as not to confuse them with the tests of the new helper, which are added in a separate class at the bottom of the module. Note that one of the tests is marked as a skip, because in the process of doing this I found what might be a bug in our grouping logic. (TL;DR, frames which are marked `-app +group` by a stacktrace rule might not be behaving as expected... depending on what you think is expected.) In any case, for now the test is skipped, and once we resolve the stacktrace rule question, we can ether turn it back on or change it.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the first step in simplifying the way we apply the stacktrace length filter to events sent to Seer. It puts in place the new machinery, but does not yet remove the old machinery, which will happen in a follow-up PR.
In the meantime, though, even though the old code is still there, the new code should supplant it, since its check comes before the existing one - meaning anything which should get filtered should be flagged by the new check and shouldn't make it through to the existing check. I used different names for the metrics so that we can make sure that's the case before we remove the existing code.
Key changes:
has_too_many_contributing_frames
to be used in all the places we send events to Seer (ingest, backfill, similar issues tab), which collects agrouping.similarity.frame_count_filter
metric, regardless of (and tagged with) the referrer._has_too_many_contributing_frames
wrapper for the above, which also records adid_call_seer
metric.grouping_info
is now split into two steps, first getting the variants and then getting grouping info from the variants, so that the variants can be fed tohas_too_many_contributing_frames
.get_stacktrace_string
tests whose names included some variation of "too many fames" have been renamed, so as not to confuse them with the tests of the new helper, which are added in a separate class at the bottom of the module.Note that one of the tests is marked as a skip, because in the process of doing this I found what might be a bug in our grouping logic. (TL;DR, frames which are marked
-app +group
by a stacktrace rule might not be behaving as expected... depending on what you think is expected.) In any case, for now the test is skipped, and once we resolve the stacktrace rule question, we can ether turn it back on or change it.