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.

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

Sentry message improvement #279

Merged
merged 8 commits into from
Mar 29, 2021
Merged

Sentry message improvement #279

Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
62375cc
sentry: add spider_name to the tags, add an empty get_tags().
wRAR Dec 11, 2020
bcbe644
sentry: add failed monitors to tags.
wRAR Dec 11, 2020
4cb5102
sentry: add reason_hash to tags.
wRAR Dec 30, 2020
bc73406
Drop reason_hash, some cleanup for tag setting.
wRAR Feb 9, 2021
796fc6d
Use slugify to get tag names for monitors.
wRAR Mar 9, 2021
c7f1e96
Merge remote-tracking branch 'origin/master' into sentry-message-impr…
wRAR Mar 10, 2021
717f30f
Change the monitor name tags logic.
wRAR Mar 10, 2021
9a0b0d0
Update the Sentry doc.
wRAR Mar 10, 2021
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
23 changes: 22 additions & 1 deletion spidermon/contrib/actions/sentry/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from __future__ import absolute_import

import logging
from itertools import groupby

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -100,12 +101,32 @@ def get_message(self):

return message

def get_tags(self, message):
tags = {
"spider_name": message.get("spider_name", ""),
"project_name": self.project_name,
}

failed_monitors = message.get("failed_monitors", [])
failed_monitors = [y.split("/") for y in failed_monitors]
for key, group in groupby(failed_monitors, key=lambda x: x[0]):
for mon in group:
try:
k = mon[1].lower().replace(" ", "_")
Copy link
Collaborator

Choose a reason for hiding this comment

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

issue: When failed method name has special characters (like á, *, etc), the tag is not applied correctly and Sentry adds an error in the notification.

invalid_chars

suggestion: It would be great if we handle these specials characters to avoid this problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ouch. I wonder how should we escape them (dropping them looks bad).

Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: Maybe we may consider using a library like python-slugify to generate proper slug for this string, removing/replacing unwanted characters. This library is already a spidermon dependency (for validation stats), so you can use it with no need to include an extra requirement to the project.

# tag keys are limited to 32 chars
tags.update({k[:32]: 1})
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: What if I have two methods with the same name (at least in the first 32 characters)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, there will be only one tag then. Note that this uses human-readable names if available so there is probably more variety. I also wonder if should strip leading "test_" to save some space.

Copy link
Collaborator

@rennerocha rennerocha Mar 1, 2021 •
edited by wRAR
Loading

Choose a reason for hiding this comment

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

suggestion: If you decorate your monitor class and/or your monitor methods with @monitors.name, the test_ part will not be added, so it won't avoid duplication. I think it is worthwhile to add a warning about this limitation in Sentry action documentation and the user can take care of not using long names for its monitor.

suggestion: Given that the format of the failed_monitors will always be <ClassName>/<MethodName> (even if the names are defined by the user) I think get_tags could be simplified to:

    def get_tags(self, message):
        tags = {
            "spider_name": message.get("spider_name", ""),
            "project_name": self.project_name,
        }

        for failed_monitor in message.get("failed_monitors", []):
            # 32 chars limited by Sentry
            key = slugify(failed_monitor.split("/")[-1])[:32]
            tags[key] = 1

        return tags

except IndexError:
pass
return tags

def send_message(self, message):

sentry_client = Client(dsn=self.sentry_dsn, environment=self.environment)

with configure_scope() as scope:
scope.set_tag("project_name", self.project_name)
tags = self.get_tags(message)
for key, val in tags.items():
scope.set_tag(key, val)

scope.set_extra("job_link", message.get("job_link", ""))
scope.set_extra("spider_name", message.get("spider_name", ""))
Expand Down