Skip to content

ref(ACI): Pull get_comparison_aggregation_value code into smaller functions #91909

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
May 20, 2025
Merged

ref(ACI): Pull get_comparison_aggregation_value code into smaller functions #91909

merged 4 commits into from
May 20, 2025

Conversation

ceorourke
Copy link
Member

Follow up to #91783 to pull code into 2 smaller functions for EAP and non EAP comparison aggregates.

@ceorourke ceorourke requested a review from a team as a code owner May 19, 2025 22:38
@ceorourke ceorourke requested a review from a team May 19, 2025 22:38
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label May 19, 2025
ceorourke
ceorourke commented May 19, 2025
View reviewed changes
src/sentry/incidents/utils/process_update_helpers.py
@@ -74,6 +75,92 @@ def get_aggregation_value_helper(subscription_update: QuerySubscriptionUpdate) -
return aggregation_value


def get_eap_aggregation_value(
Copy link
Member Author

Choose a reason for hiding this comment

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

This code was added in https://github.com/getsentry/sentry/pull/81948/files#diff-d262481ba0aaae3473d14f62d3cbb554e099bda1093454b7935890139f3fe5a8 so it seems to be EAP related

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 yeah - seems alright to move into a method like this.

cc @shruthilayaj just incase to confirm

saponifi3d
saponifi3d approved these changes May 19, 2025
View reviewed changes
Copy link
Contributor

@saponifi3d saponifi3d left a comment •
edited
Loading

Choose a reason for hiding this comment

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

lgtm since it's just moving the code into methods. 🎉 thanks for the refactor.

src/sentry/incidents/utils/process_update_helpers.py
@@ -74,6 +75,92 @@ def get_aggregation_value_helper(subscription_update: QuerySubscriptionUpdate) -
return aggregation_value


def get_eap_aggregation_value(
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 yeah - seems alright to move into a method like this.

cc @shruthilayaj just incase to confirm

src/sentry/incidents/utils/process_update_helpers.py
alert_rule_id: int | None = None,
) -> float | None:
try:
# TODO: determine whether we need to include the subscription query_extra here
Copy link
Contributor

Choose a reason for hiding this comment

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

unrelated to this pr -- but anyone have context over this comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's from an old Nathan PR 028e31a#diff-d262481ba0aaae3473d14f62d3cbb554e099bda1093454b7935890139f3fe5a8 but I don't know the context, do you remember?

@codecov Codecov
Copy link

codecov bot commented May 19, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 78.57143% with 6 lines in your changes missing coverage. Please review.

⚠️ Parser warning

The parser emitted a warning. Please review your JUnit XML file:

Warning while parsing testcase attributes: Limit of string is 1000 chars, for name, we got 2083 at 1:157236 in /home/runner/work/sentry/sentry/.artifacts/pytest.junit.xml
Files with missing lines Patch % Lines
...c/sentry/incidents/utils/process_update_helpers.py 78.57% 6 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #91909      +/-   ##
==========================================
- Coverage   87.63%   87.62%   -0.01%     
==========================================
  Files       10356    10356              
  Lines      587133   587148      +15     
  Branches    22585    22585              
==========================================
+ Hits       514506   514515       +9     
- Misses      72199    72205       +6     
  Partials      428      428

@ceorourke ceorourke merged commit e1c3676 into master May 20, 2025
60 checks passed
@ceorourke ceorourke deleted the ceorourke/ref-get_comparison_aggregation_value branch May 20, 2025 16:28
@sentry-io Sentry.io
Copy link

sentry-io bot commented May 20, 2025 •
edited
Loading

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ InvalidSearchQuery: Invalid value '['AWAY-RESORTS-WEBSITE-QJ']' for 'issue:' filter query_subscription_consumer_process_message View Issue
  • ‼️ IncompatibleMetricsQuery: component is not a tag in the metrics dataset query_subscription_consumer_process_message View Issue
  • ‼️ SnubaError: HTTPConnectionPool(host='snuba-api', port=80): Read timed out. (read timeout=30) query_subscription_consumer_process_message View Issue
  • ‼️ IncompatibleMetricsQuery: HTTPConnectionPool(host='snuba-api', port=80): Read timed out. (read timeout=30) query_subscription_consumer_process_message View Issue
  • ‼️ SnubaError: HTTPConnectionPool(host='snuba-api', port=80): Max retries exceeded with url: /generic_metrics/snql (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x796071bcf410>: Failed to establish a new connection: [Errno 111] Connection... query_subscription_consumer_process_message View Issue

Did you find this useful? React with a 👍 or 👎

andrewshie-sentry pushed a commit that referenced this pull request May 20, 2025
@ceorourke @andrewshie-sentry
ref(ACI): Pull get_comparison_aggregation_value code into smaller fun…
ecb9e0f 
…ctions (#91909)

Follow up to #91783 to pull code
into 2 smaller functions for EAP and non EAP comparison aggregates.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saponifi3d saponifi3d saponifi3d approved these changes

@shruthilayaj shruthilayaj shruthilayaj approved these changes

Assignees
No one assigned
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ceorourke @saponifi3d @shruthilayaj