Skip to content
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

wip(eap): http response rate function #85662

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

DominikB2014
Copy link
Contributor

@DominikB2014 DominikB2014 commented Feb 21, 2025

Work for #81750

  1. Adds the ability to define custom functions (formulas)
  2. Implement the http_response_rate function, which is used in insights

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Feb 21, 2025
Copy link

codecov bot commented Feb 21, 2025

Codecov Report

Attention: Patch coverage is 95.83333% with 5 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/sentry/search/eap/resolver.py 88.57% 4 Missing ⚠️
src/sentry/search/eap/columns.py 97.56% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #85662       +/-   ##
===========================================
+ Coverage   42.22%   87.90%   +45.68%     
===========================================
  Files        9665     9697       +32     
  Lines      548129   550042     +1913     
  Branches    21345    21345               
===========================================
+ Hits       231453   483540   +252087     
+ Misses     316366    66192   -250174     
  Partials      310      310               

Comment on lines +700 to +701
if function in self.definitions.functions:
function_definition = self.definitions.functions[function]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a future refactor, we can rename self.definitions.functions to self.definitions.aggregates

@DominikB2014 DominikB2014 marked this pull request as ready for review February 26, 2025 16:06
@DominikB2014 DominikB2014 requested a review from a team as a code owner February 26, 2025 16:06
@DominikB2014 DominikB2014 requested a review from wmak February 26, 2025 16:07
@DominikB2014
Copy link
Contributor Author

DominikB2014 commented Feb 26, 2025

@wmak I addressed your comments!
I slapped all the formulas in one file for now.
In future PRs, i'm planning to splitting up the span_columns as we discussed and address the TODO comments.

Lmk if that works for you!



@dataclass
class FormulaDefinition:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we have this inherit from FunctionDefinition so we're not repeating everything?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's some class variables that don't apply to both. For example for FormulaDefinition, internal_type doesnt isn't relevant.

I'm thinking when we do the split, we could have a base class FunctionDefinition, and then AggregateDefinition and FormulaDefinition inherit from that?

Copy link
Member

@wmak wmak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall in the right direction, I'm mostly concerned that we're sorta hacking the argument resolution currently.

@DominikB2014
Copy link
Contributor Author

DominikB2014 commented Feb 26, 2025

Overall in the right direction, I'm mostly concerned that we're sorta hacking the argument resolution currently.

👍 thanks for the review, I don't mind spending some more time getting the args working properly in this PR

@DominikB2014
Copy link
Contributor Author

DominikB2014 commented Feb 28, 2025

@wmak I cleaned up the argument list a bit more. Since you looked there several changes

  1. ArgumentDefinition optionally takes in is_attribute and validator
  2. resolve_aggregate iterates over the arguments we now
    a. checks against the argument validator, if the argument validator fails the query fails
    b. If the argument is not an attribute, we skip resolve_attribute and just parse the attribute directly based on the argument_types (We don't have a use case for multiple argument types in formulas yet, so i didn't handle it in this PR)
  3. Now that the FormulaDefinition defines all the validation, there is no need to validate in the formula_resolver itself
  4. Now resolve_aggregate parses the argument, the formula_resolver no longer needs to do any parses and assumes the params are as expected
  5. A couple variable name changes so things make more sense (previously everything assumed an arg was an attribute)

@DominikB2014 DominikB2014 requested a review from wmak February 28, 2025 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants