Skip to content

Adding Handled/Unhandled payload properties#126

Merged
kattrali merged 14 commits into
masterfrom
handled-unhandled
Sep 25, 2017
Merged

Adding Handled/Unhandled payload properties#126
kattrali merged 14 commits into
masterfrom
handled-unhandled

Conversation

@Cawllec
Copy link
Copy Markdown
Contributor

@Cawllec Cawllec commented Sep 13, 2017

  • Adds defaultSeverity, unhandled, and severityReasons payload properties to support upcoming handled/unhandled functionality
  • Above properties added through **options parameters
  • System error handler, log level handlers, and middleware error handlers set up with severity reasons
    references PLAT_209

Comment thread bugsnag/wsgi/middleware.py Outdated

def __init__(self, application, environ, start_response):
self.environ = environ
self.severity_reason = {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be a constant rather than a member on the middleware

Comment thread bugsnag/client.py Outdated
"No API key configured, couldn't notify")
return

notification.default_severity = initial_default_severity
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I dont think we need to reset any changes to these variables in here. If people change them then they are in expert mode and thats fine.

Comment thread bugsnag/celery/__init__.py Outdated
context=sender.name,
extra_data=task)
extra_data=task,
unhandled=True,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this not implied because auto notify?

@Cawllec
Copy link
Copy Markdown
Contributor Author

Cawllec commented Sep 15, 2017

I've updated the notify default-severity check, that shouldn't automatically set default_severity to false when unhandled calls to it are made. I've taken out the overriding of severity_reason and unhandled, but this is a pattern in some of the other notifiers (whether through making them protected properties or by overriding them if necessary) which may need discussion

Cawllec and others added 4 commits September 21, 2017 14:32
Separating capture from callback reasons indicates more clearly to users
where the severity was changed and why. It has a lower precedence than
callbacks which can still override the severity.
@kattrali kattrali merged commit 9933dce into master Sep 25, 2017
@kattrali kattrali deleted the handled-unhandled branch September 25, 2017 12:40
Copy link
Copy Markdown
Contributor

@snmaynard snmaynard left a comment

Choose a reason for hiding this comment

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

These review comments should be addressed even though it's been merged already

Comment thread bugsnag/client.py
'type': 'userCallbackSetSeverity'
}
else:
notification.severity_reason = initial_reason
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we do this here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This enforces that users cannot overwrite the severity_reason in callbacks or middleware. I think we previously said it wouldn't matter if someone chose to do so, but we've added it as one of our testing parameters and are enforcing the same thing across notifiers.

Comment thread bugsnag/client.py
self.client = client
self.options = options
if 'severity' in options:
options['severity_reason'] = dict(type='contextSpecifiedSeverity')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this be userContextSetSeverity?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This one seems to already have been fixed

Comment thread bugsnag/handlers.py
'meta_data': {},
'unhandled': False,
'severity_reason': {
'type': 'logs',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be log

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants