Skip to content

ref: Deprecate get_legacy_message #16310

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

Merged
merged 2 commits into from
Jan 10, 2020
Merged

ref: Deprecate get_legacy_message #16310

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7b5c909
ref: Deprecate get_legacy_message
lynnagara Jan 7, 2020
3627285
fix test
lynnagara Jan 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 0 additions & 10 deletions src/sentry/models/event.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,16 +118,6 @@ def interfaces(self):
def get_interface(self, name):
return self.interfaces.get(name)

def get_legacy_message(self):
# TODO: This is only used in the pagerduty plugin. We should use event.title
# there and remove this function once users have been notified, since PD
# alert routing may be based off the message field.
return (
get_path(self.data, "logentry", "formatted")
or get_path(self.data, "logentry", "message")
or self.message
)

def get_event_type(self):
"""
Return the type of this event.
Expand Down
5 changes: 4 additions & 1 deletion src/sentry_plugins/pagerduty/plugin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,10 @@ def notify_users(self, group, event, fail_silently=False, **kwargs):
if not self.is_configured(group.project):
return

description = event.get_legacy_message()[:1024]
# TODO: This should eventually be event.title in line with other plugins.
# However, we should notify users first, since PD alert routing may be
# based off the message field.
description = (event.real_message or event.message)[:1024]
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference in content between the logentry attribute you were using before and real_message/event.message

Copy link
Member Author

Choose a reason for hiding this comment

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

There shouldn't be any difference in content here, this is just a refactor to clean up the event class and move the Pagerduty specific message into the plugin

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please expand on the "shouldn't be any difference" ? You are now fetching a different field from the event. Where was the old logentry populated from ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean if you look at the implementation of real_message vs get_legacy_message(), they are almost exactly the same except that legacy message falls back to event.message, which is what we have done here.

Copy link
Contributor

Choose a reason for hiding this comment

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

damn you are right, I was looking at the wrong method.


tags = dict(event.tags)
details = {
Expand Down
2 changes: 1 addition & 1 deletion tests/sentry_plugins/pagerduty/test_plugin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ def test_simple_notification(self):
"datetime": event.datetime.strftime("%Y-%m-%dT%H:%M:%S.%fZ"),
},
"service_key": "abcdef",
"description": event.get_legacy_message(),
"description": event.real_message,
}

def test_no_secrets(self):
Expand Down