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

feat(escalating-issues): Detect when an issue starts escalating #47843

Merged
merged 4 commits into from
Apr 25, 2023

Conversation

jangjodi
Copy link
Member

@jangjodi jangjodi commented Apr 24, 2023

WOR-2762

Acceptance Criteria:

  • this should happen in the process_snoozes pipeline step
    • this step currently handles the ignored logic
  • Read the forecast from the cache first and then from the nodestore
    • Cache it if we had to hit the nodestore
  • Check if the current event occurence is greater than the forecast
    • If yes, then:
      • we should update the GroupSubStatus to Escalating
      • we should update the GroupStatus to UNRESOLVED
      • call the add_group_to_inbox method and set to the GroupInboxReason.ESCALATING

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Apr 24, 2023
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Apr 24, 2023
@jangjodi jangjodi removed the Scope: Frontend Automatically applied to PRs that change frontend components label Apr 24, 2023
@getsentry getsentry deleted a comment from github-actions bot Apr 24, 2023
@jangjodi jangjodi marked this pull request as ready for review April 24, 2023 18:44
@jangjodi jangjodi requested review from a team April 24, 2023 18:44
@codecov
Copy link

codecov bot commented Apr 25, 2023

Codecov Report

Merging #47843 (4bc9099) into master (1127c14) will increase coverage by 3.31%.
The diff coverage is 87.80%.

❗ Current head 4bc9099 differs from pull request most recent head 417dc8e. Consider uploading reports for the commit 417dc8e to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #47843      +/-   ##
==========================================
+ Coverage   77.50%   80.81%   +3.31%     
==========================================
  Files        4759     4763       +4     
  Lines      201207   201371     +164     
  Branches    11594    11594              
==========================================
+ Hits       155948   162745    +6797     
+ Misses      45003    38370    -6633     
  Partials      256      256              
Impacted Files Coverage Δ
src/sentry/tasks/post_process.py 92.32% <28.57%> (+16.53%) ⬆️
src/sentry/issues/escalating.py 100.00% <100.00%> (ø)
src/sentry/snuba/referrer.py 100.00% <100.00%> (ø)

... and 399 files with indirect coverage changes

cache.set(forecast_cache_key, escalating_forecast, forecast_cache_duration)

# Check if current event occurance is greater than forecast for today's date
group_daily_count = get_group_daily_count(group.project.id, group.id)
Copy link
Member

Choose a reason for hiding this comment

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

Let's keep an eye on this and make sure it doesn't add too much load to snuba. If it does, one option we have is to only check is_escalating for an issue at most once per minute.

Copy link
Member

Choose a reason for hiding this comment

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

(This might be a lot more important when we start auto archiving issues as ignore until escalating)

return
job["has_reappeared"] = False
return

with metrics.timer("post_process.process_snoozes.duration"):
Copy link
Member

Choose a reason for hiding this comment

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

It would be great if we could refactor this block into a new function in a different PR.

forecast_cache_duration = (
(escalating_forecast[0] + timedelta(days=ONE_WEEK_DURATION)).date() - date_now
).total_seconds()
cache.set(forecast_cache_key, escalating_forecast, forecast_cache_duration)
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking that we may want to do this as part of the save call in here:

def save(self) -> None:
nodestore.set(
self.build_storage_identifier(self.project_id, self.group_id),
self.to_dict(),
ttl=timedelta(TWO_WEEKS_IN_DAYS_TTL),
)

escalating_forecast = cache.get(forecast_cache_key)
date_now = datetime.now().date()
if escalating_forecast is None:
escalating_forecast = EscalatingGroupForecast.fetch(group.project.id, group.id)
Copy link
Member

Choose a reason for hiding this comment

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

You can add the getting from cache directly to the fetch method, thus, simplifying the is_escalating method.

__all__ = ["query_groups_past_counts", "parse_groups_past_counts"]

REFERRER = "sentry.issues.escalating"
ELEMENTS_PER_SNUBA_PAGE = 10000 # This is the maximum value for Snuba
# The amount of data needed to generate a group forecast
BUCKETS_PER_GROUP = 7 * 24
ONE_WEEK_DURATION = 7
IS_ESCALATING_REFERRER = "sentry.issues.escalating.is_escalating"
Copy link
Member

Choose a reason for hiding this comment

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

I realized it is better that we import the referrers from referrer.py.

@github-actions github-actions bot locked and limited conversation to collaborators May 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants