-
-
Notifications
You must be signed in to change notification settings - Fork 62
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
fix(eap): fix bug with data_present #6726
Conversation
❌ 1 Tests Failed:
View the top 1 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
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.
LGTM
snuba/web/rpc/common/aggregation.py
Outdated
@@ -46,7 +46,7 @@ class ExtrapolationContext(ABC): | |||
sample_count: int | |||
|
|||
@property | |||
def extrapolated_data_present(self) -> bool: | |||
def data_present(self) -> bool: |
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: should this be renamed for is_data_present
since it returns an answer to this question?
snuba/web/rpc/common/aggregation.py
Outdated
return GenericExtrapolationContext( | ||
value=value, | ||
confidence_interval=None, | ||
average_sample_rate=0, | ||
sample_count=0, | ||
sample_count=sample_count, |
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.
I am wondering if this return
can be collapsed with the return
on L130 for better readability. All we are checking is if confidence_interval
is None
and if so, we pass None
.
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.
yeah you are right, it can be collapsed, nice catch
Fixes https://github.com/getsentry/eap-planning/issues/144
Additional Context
When we perform aggregations over attributes, the function is converted into function_nameIf (e.g. countIf) which returns a default value if no rows match the condition. This means that there is no way to distinguish between an aggregate result that is 0 because it's for example a sum of values that add up to 0, and a result that is 0 because no values matched the given condition. To deal with this, we compute the number of events being aggregated even when we aren't extrapolating so we can determine if data was present or not.