Skip to content

Add support for add_note() #3056

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed

Conversation

arjenzorgdoc
Copy link
Contributor

See issue #3050

This adds support for __notes__ and adds them to the exception value.

@antonpirker antonpirker added the Component: SDK Core Dealing with the core of the SDK label Jun 7, 2024
@antonpirker antonpirker added this to the SDK Core milestone Jun 7, 2024
@antonpirker antonpirker removed this from the SDK Core milestone Jun 20, 2024
getattr(exc_value, "message", "")
or getattr(exc_value, "detail", "")
or safe_str(exc_value)
)
notes = getattr(exc_value, "__notes__", [])
if notes:
value = "\n".join([value] + [safe_str(note) for note in notes])
Copy link
Member

Choose a reason for hiding this comment

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

I am unsure whether it makes sense to add the note to the exception message; perhaps setting this as extra data would be better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I responded in the pull request conversation.

I think the notes add context that developers expect to be shown directly with the error message. That is what CPython does in the console.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this safe_str stuff here, notes should anyways just be strings. This is guaranteed when using add_note. Although you correctly point out in your test that it is possible to add non-string values to the __notes__ list manually, this is likely a misuse of __notes__, and is not something we need to support; although, we should still not crash if there is an invalid value in the __notes__.

I would suggest we just filter out any non-string values from the __notes__.

@getsantry
Copy link

getsantry bot commented Aug 16, 2024

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you remove the label Waiting for: Community, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@getsantry getsantry bot added the Stale label Aug 16, 2024
@getsantry
Copy link

getsantry bot commented Sep 7, 2024

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you remove the label Waiting for: Community, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@arjenzorgdoc
Copy link
Contributor Author

The use case for the notes is to add text to the exception massage.

I think notes_new() is the new way to do notes_old():

def failure():
    raise ValueError("Nope.")


MESSAGE = """\
Nope. It's wrong!
See https://example.com/why-nope.html for more info
"""


def notes_old():
    try:
        failure()
    except Exception as e:
        raise RuntimeError(f"{e}\n{MESSAGE}\n") from e


def notes_new():
    try:
        failure()
    except Exception as e:
        e.add_note(MESSAGE)
        raise

notes_old() renders like this:

image

I tried to have notes_new() render the same.

BTW python cli renders the notes after the Exception message as well:

  File ".../sentry_notes.py", line 7, in failure
    raise ValueError("Nope.")
ValueError: Nope.
Nope. It's wrong!
See https://example.com/why-nope.html for more info

@getsantry
Copy link

getsantry bot commented Sep 29, 2024

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you remove the label Waiting for: Community, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@arjennienhuis
Copy link

def get_foo(direction):
    D = dict(up=6, down=4)
    try:
        return D[direction]
    except KeyError as e:
        e.add_note(f"{direction!r} is not a valid direction")
        raise
Traceback (most recent call last):
  File ".../test.py", line 9, in <module>
    get_foo("left")
  File ".../test.py", line 4, in get_foo
    return D[direction]
           ~^^^^^^^^^^^
KeyError: 'left'
'left' is not a valid direction

I think sentry should render this as:

KeyError

'left'
'left' is not a valid direction

@arjenzorgdoc arjenzorgdoc force-pushed the add-notes-support branch 2 times, most recently from 4bcaf56 to 194ef32 Compare September 30, 2024 07:34
Copy link

codecov bot commented Oct 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.25%. Comparing base (205591e) to head (9a04d67).
Report is 20 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3056      +/-   ##
==========================================
- Coverage   84.30%   84.25%   -0.06%     
==========================================
  Files         133      133              
  Lines       13902    13906       +4     
  Branches     2933     2935       +2     
==========================================
- Hits        11720    11716       -4     
- Misses       1444     1455      +11     
+ Partials      738      735       -3     
Files with missing lines Coverage Δ
sentry_sdk/utils.py 86.64% <100.00%> (+0.31%) ⬆️

... and 4 files with indirect coverage changes

@szokeasaurusrex
Copy link
Member

Even though the PR says it can be edited by maintainers, I am getting a 403 when I try to push to this PR branch. So, I am closing this PR in favor of #3620

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Component: SDK Core Dealing with the core of the SDK Waiting for: Community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants