-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(errors): Events API backend - Add error upsampling aggregation functions #95940
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
Conversation
🚨 Warning: This pull request contains Frontend and Backend changes! It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently. Have questions? Please ask in the |
can we separate the FE changes into a different PR from the BE ones? |
yea Ill break it apart into FE and BE |
… flag Allow projects with error upsampling to use new sample_count(), sample_eps() and sample_epm() function columns in Discover, returning the non extrapolated versions of these functions.
6b554d2
to
f63e995
Compare
@roggenkemper Separated it, the FE PR is now in: #95946 |
result[f"{function}()"] = result[function] | ||
del result[function] | ||
|
||
for function in upsampling_affected_functions: |
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.
could these loops be combined?
❌ 3 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
fix issues with double conversion and moved logic into helper
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.
Bug: Upsampling Function Data Corruption
The convert_fields_for_upsampling
function has two issues: it can lead to incorrect data in query results and fails to convert metadata correctly for empty results. Specifically, if both a standard function (e.g., count()
) and its upsampled equivalent (e.g., upsampled_count()
) are present, the standard function's key will incorrectly hold the upsampled value after conversion. Additionally, fields_meta
keys are not converted when a query returns no data, as the conversion logic relies on keys being present in the data, resulting in incorrect field names in the metadata.
src/sentry/api/helpers/error_upsampling.py#L41-L62
sentry/src/sentry/api/helpers/error_upsampling.py
Lines 41 to 62 in 54fddec
# Collect keys that need conversion and exist in data | |
all_present_keys: set[str] = set() | |
for result in data: | |
all_present_keys.update(result.keys()) | |
# Filter the pre-ordered list to only include keys actually present | |
keys_to_convert = [key for key in _ORDERED_CONVERSION_KEYS if key in all_present_keys] | |
# Apply conversions to data | |
for result in data: | |
for original_key in keys_to_convert: | |
if original_key in result: | |
converted_key = _FUNCTION_KEY_CONVERSIONS[original_key] | |
result[converted_key] = result[original_key] | |
del result[original_key] | |
# Apply conversions to fields_meta | |
for original_key in keys_to_convert: | |
if original_key in fields_meta: | |
converted_key = _FUNCTION_KEY_CONVERSIONS[original_key] | |
fields_meta[converted_key] = fields_meta[original_key] | |
del fields_meta[original_key] |
Was this report helpful? Give feedback by reacting with 👍 or 👎
def test_sample_count_with_allowlisted_project(self): | ||
"""Test that sample_count() returns raw sample count (not upsampled) for allowlisted projects.""" | ||
# Set up allowlisted project | ||
with self.options({"issues.client_error_sampling.project_allowlist": [self.project.id]}): |
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.
Should this use the flagpole definition now
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.
hmm yea I can probably change the backend to use the flagpole instead of the existing option, but I am under some time pressure, that is a cleanup I can do and test afterwards, I don't need to do that yet really, I basically now have an option for the backend and a flappole for the frontend, and ill keep it like that for now and unite them later.
…unctions (#95940) Events API now supports new sample_count(), sample_eps() and sample_epm() function columns in Discover for error upsampling projects, which return the original non extrapolated versions of these functions.
Allow projects with error upsampling to use new sample_count(), sample_eps() and
sample_epm() function columns in Discover, returning the non extrapolated versions of these functions.