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

remove deprecated backend code #2502

Merged
merged 11 commits into from
Jul 12, 2023
Merged

Conversation

joeyorlando
Copy link
Contributor

@joeyorlando joeyorlando commented Jul 11, 2023

What this PR does

See more details comments alongside the code.

Regarding frontend changes, the main changes in this PR are to remove unused fields on the Team interface + unused methods on the Team model.

Checklist

  • Unit, integration, and e2e (if applicable) tests updated
  • Documentation added (or pr:no public docs PR label added if not required) (N/A)
  • CHANGELOG.md updated (or pr:no changelog PR label added if not required) (N/A)

@joeyorlando joeyorlando added pr:no changelog pr:no public docs Added to a PR that does not require public documentation updates labels Jul 11, 2023
@joeyorlando joeyorlando requested review from a team July 11, 2023 15:32
@@ -31,7 +31,7 @@ def resolve_alert_group_by_source_if_needed(alert_group_pk):
alert_group.save(update_fields=["resolved_by"])
if alert_group.resolved_by == alert_group.NOT_YET_STOP_AUTORESOLVE:
return "alert_group is too big to auto-resolve"
print("YOLO")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines -719 to -749
def resolve_by_archivation(self):
AlertGroupLogRecord = apps.get_model("alerts", "AlertGroupLogRecord")
# if incident was silenced, unsilence it without starting escalation
if self.silenced:
self.un_silence()
self.log_records.create(
type=AlertGroupLogRecord.TYPE_UN_SILENCE,
silence_delay=None,
reason="Resolve by archivation",
)
self.archive()
self.stop_escalation()
if not self.resolved:
self.resolve(resolved_by=AlertGroup.ARCHIVED)

log_record = self.log_records.create(type=AlertGroupLogRecord.TYPE_RESOLVED)

logger.debug(
f"send alert_group_action_triggered_signal for alert_group {self.pk}, "
f"log record {log_record.pk} with type '{log_record.get_type_display()}', action source: archivation"
)

alert_group_action_triggered_signal.send(
sender=self.resolve_by_archivation,
log_record=log_record.pk,
action_source=None,
)

for dependent_alert_group in self.dependent_alert_groups.all():
dependent_alert_group.resolve_by_archivation()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolve_by_archivation was only ever invoked in the following two spots:

  • CheckAlertIsUnarchivedMixin.check_alert_is_unarchived: this method seemed to be deprecated because the logic inside the method was as such:
def check_alert_is_unarchived(self, slack_team_identity, payload, alert_group, warning=True):
    alert_group_is_unarchived = alert_group.started_at.date() > self.organization.archive_alerts_from
    if not alert_group_is_unarchived:
        if warning:
            warning_text = "Action is impossible: the Alert is archived."
            self.open_warning_window(payload, warning_text)
        if not alert_group.resolved or not alert_group.is_archived:
            alert_group.resolve_by_archivation()
    return alert_group_is_unarchived

organization.archive_alerts_from is always set to 1970-01-01 (only value that exists in our production database) so therefore the above logic will never invoke AlertGroup.resolve_by_archivation

  • in the resolve_archived_incidents_for_organization Celery task

this celery task was only ever queued by CurrentOrganizationSerializer.update:

def update(self, instance, validated_data):
    current_archive_date = instance.archive_alerts_from
    archive_alerts_from = validated_data.get("archive_alerts_from")

    result = super().update(instance, validated_data)
    if archive_alerts_from is not None and current_archive_date != archive_alerts_from:
        if current_archive_date > archive_alerts_from:
            unarchive_incidents_for_organization.apply_async(
                (instance.pk,),
            )
        resolve_archived_incidents_for_organization.apply_async(
            (instance.pk,),
        )

    return result

archive_alerts_from will always be None here

Comment on lines -18 to -26
class CustomDateField(fields.TimeField):
def to_internal_value(self, data):
try:
archive_datetime = datetime.datetime.fromisoformat(data).astimezone(pytz.UTC)
except (TypeError, ValueError):
raise serializers.ValidationError({"archive_alerts_from": ["Invalid date format"]})
if archive_datetime.date() >= timezone.now().date():
raise serializers.ValidationError({"archive_alerts_from": ["Invalid date. Date must be less than today."]})
return archive_datetime
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not refrenced anywhere

@@ -109,10 +82,6 @@ def get_banner(self, obj):
)[0]
return banner.json_value

def get_limits(self, obj):
user = self.context["request"].user
return obj.notifications_limit_web_report(user)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

limits is not used by the web UI

@@ -122,44 +91,9 @@ def get_env_status(self, obj):
phone_provider_config = get_phone_provider().flags
return {
"telegram_configured": telegram_configured,
"twilio_configured": phone_provider_config.configured, # keep for backward compatibility
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not referenced anywhere (search)

Comment on lines -129 to -145
def get_stats(self, obj):
if isinstance(obj.cached_seconds_saved_by_amixr, int):
verbal_time_saved_by_amixr = humanize.naturaldelta(
datetime.timedelta(seconds=obj.cached_seconds_saved_by_amixr)
)
else:
verbal_time_saved_by_amixr = None

result = {
"grouped_percent": obj.cached_grouped_percent,
"alerts_count": obj.cached_alerts_count,
"noise_reduction": obj.cached_noise_reduction,
"average_response_time": humanize.naturaldelta(obj.cached_average_response_time),
"verbal_time_saved_by_amixr": verbal_time_saved_by_amixr,
}

return result
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not referenced anywhere, or consumed by the web UI

@@ -83,7 +82,6 @@
GetChannelVerificationCode.as_view(),
name="api-get-channel-verification-code",
),
optional_slash_path("current_subscription", SubscriptionView.as_view(), name="subscription"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this endpoint is not referenced/consumed by the web plugin

Comment on lines -34 to -51
def notifications_limit_web_report(self, user):
limits_to_show = []
left = self._calculate_phone_notifications_left(user)
limit = self._phone_notifications_limit
limits_to_show.append({"limit_title": "Phone Calls & SMS", "total": limit, "left": left})
show_limits_warning = left <= limit * 0.2 # Show limit popup if less than 20% of notifications left

warning_text = (
f"You{'' if left == 0 else ' almost'} have exceeded the limit of phone calls and sms:"
f" {left} of {limit} left."
)

return {
"period_title": "Daily limit",
"limits_to_show": limits_to_show,
"show_limits_warning": show_limits_warning,
"warning_text": warning_text,
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

was only used by CurrentOrganizationSerializer.get_limits, which was not used, thereby this can be removed as well

Comment on lines -147 to -161
def update(self, instance, validated_data):
current_archive_date = instance.archive_alerts_from
archive_alerts_from = validated_data.get("archive_alerts_from")

result = super().update(instance, validated_data)
if archive_alerts_from is not None and current_archive_date != archive_alerts_from:
if current_archive_date > archive_alerts_from:
unarchive_incidents_for_organization.apply_async(
(instance.pk,),
)
resolve_archived_incidents_for_organization.apply_async(
(instance.pk,),
)

return result
Copy link
Contributor Author

Choose a reason for hiding this comment

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

see this comment here for a breakdown of why we can get rid of this

Comment on lines -165 to -174
class CheckAlertIsUnarchivedMixin:
def check_alert_is_unarchived(self, slack_team_identity, payload, alert_group, warning=True):
alert_group_is_unarchived = alert_group.started_at.date() > self.organization.archive_alerts_from
if not alert_group_is_unarchived:
if warning:
warning_text = "Action is impossible: the Alert is archived."
self.open_warning_window(payload, warning_text)
if not alert_group.resolved or not alert_group.is_archived:
alert_group.resolve_by_archivation()
return alert_group_is_unarchived
Copy link
Contributor Author

Choose a reason for hiding this comment

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

see this comment here for why this class/method is always returning True, ie. we can get rid of it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the four celery tasks that're being removed here are:

  • not referenced anywhere
  • don't see any logs/traces of them over the past 7 days in production

Comment on lines +15 to +26
migrations.RemoveField(
model_name='alertgroup',
name='active_cache_for_web_calculation_id',
),
migrations.RemoveField(
model_name='alertgroup',
name='estimate_escalation_finish_time',
),
migrations.RemoveField(
model_name='alertgroup',
name='cached_render_for_web',
),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The DROP COLUMN operation supports the "Instant" algorithm in MySQL v8.0 so this should be a very quick operation (docs)

@@ -364,9 +364,6 @@ def enrich(self, alert_groups):
alert_group_pks = [alert_group.pk for alert_group in alert_groups]
queryset = AlertGroup.all_objects.filter(pk__in=alert_group_pks).order_by("-pk")

# do not load cached_render_for_web as it's deprecated and can be very large
queryset = queryset.defer("cached_render_for_web")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was the only reference to cached_render_for_web

Comment on lines -81 to -98
@action
async getTelegramVerificationCode(pk: Team['pk']) {
const response = await makeRequest(`/teams/${pk}/get_telegram_verification_code/`, {
withCredentials: true,
});

return response;
}

@action
async unlinkTelegram(pk: Team['pk']) {
const response = await makeRequest(`/teams/${pk}/unlink_telegram/`, {
method: 'POST',
withCredentials: true,
});

return response;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@iskhakov
Copy link
Contributor

great job!

@joeyorlando joeyorlando enabled auto-merge July 12, 2023 05:23
@joeyorlando joeyorlando disabled auto-merge July 12, 2023 05:29
@joeyorlando joeyorlando merged commit 385e137 into dev Jul 12, 2023
@joeyorlando joeyorlando deleted the jorlando/remove-unused-celery-tasks branch July 12, 2023 06:07
github-merge-queue bot pushed a commit that referenced this pull request Jul 18, 2023
…_objects (#2524)

# What this PR does

This is a follow up to #2502 which started to remove logic to
"archiving" alert groups. This PR:
- removes all references to `AlertGroup.is_archived` and marks the
column as deprecated. We will remove it in the next release
- removes the `AlertGroup.unarchived_objects` `Manager`
- renames the `AlertGroup.all_objects` `Manager` to `AlertGroup.objects`

## Checklist

- [x] Unit, integration, and e2e (if applicable) tests updated
- [ ] Documentation added (or `pr:no public docs` PR label added if not
required)
- [x] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not
required)
brojd pushed a commit that referenced this pull request Sep 18, 2024
# What this PR does

See more details comments alongside the code.

Regarding frontend changes, the main changes in this PR are to remove
unused fields on the `Team` interface + unused methods on the `Team`
model.

## Checklist

- [x] Unit, integration, and e2e (if applicable) tests updated
- [ ] Documentation added (or `pr:no public docs` PR label added if not
required) (N/A)
- [ ] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not
required) (N/A)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:no public docs Added to a PR that does not require public documentation updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants