Sentry message improvement #279
Conversation
Codecov Report
@@ Coverage Diff @@
## master #279 +/- ##
==========================================
- Coverage 73.66% 73.02% -0.65%
==========================================
Files 68 68
Lines 2954 2980 +26
Branches 319 323 +4
==========================================
Hits 2176 2176
- Misses 717 743 +26
Partials 61 61
Continue to review full report at Codecov.
|
| for mon in group: | ||
| try: | ||
| k = mon[1].lower().replace(" ", "_") | ||
| # 32 chars undocumented tag key length |
There was a problem hiding this comment.
question: Is this a limit from Sentry (not allowing tags with more than 32 chars)? If so, maybe it is worthy to include that information in the documentation of the action.
There was a problem hiding this comment.
Actually, it's now documented:
Tag keys are limited to 32 characters.
Tag values are limited to 200 characters.
| for key, group in groupby(failed_monitors, key=lambda x: x[0]): | ||
| for mon in group: | ||
| try: | ||
| k = mon[1].lower().replace(" ", "_") |
There was a problem hiding this comment.
issue: When failed method name has special characters (like á, *, etc), the tag is not applied correctly and Sentry adds an error in the notification.
suggestion: It would be great if we handle these specials characters to avoid this problem.
There was a problem hiding this comment.
Ouch. I wonder how should we escape them (dropping them looks bad).
There was a problem hiding this comment.
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.
| try: | ||
| k = mon[1].lower().replace(" ", "_") | ||
| # 32 chars undocumented tag key length | ||
| tags.update({k[:32]: 1}) |
There was a problem hiding this comment.
question: What if I have two methods with the same name (at least in the first 32 characters)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| @@ -100,15 +102,47 @@ def get_message(self): | |||
|
|
|||
| return message | |||
|
|
|||
| @staticmethod | |||
| def get_tags(message): | |||
There was a problem hiding this comment.
suggestion: We don't have any tests for this action. It would be good to have at least some tests for get_tags method.
There was a problem hiding this comment.
I don't think we have tests for anything in this module, what do you suggest?
There was a problem hiding this comment.
Almost good to merge, just minor changes to make it work in a more general way):
- Add slugify to generate better names for the tags (excluding unicode invalid values)
- Document
get_tagsmethod informing which tags are included to the notification and adding an warning about the 32 chars limits for the tag name.
As discussed with Renne before, this is a small batch of improvements for
SendSentryMessagecopied from internal SH projects and already used there for some time. I've split them into several commits:I omitted another thing: setting the fingerprint to
[spider_name, reason_hash](so that the reports are only grouped into one if the list of failed monitors matches) as that's backward incompatible and probably needs either a setting or an easy way to do this in a subclass.