Skip to content

Align optional parameters for span related to exceptions, add exception.escaped for record_exception#1365

Merged
lzchen merged 12 commits intoopen-telemetry:masterfrom
lzchen:exp
Nov 13, 2020
Merged

Align optional parameters for span related to exceptions, add exception.escaped for record_exception#1365
lzchen merged 12 commits intoopen-telemetry:masterfrom
lzchen:exp

Conversation

@lzchen
Copy link
Copy Markdown
Contributor

@lzchen lzchen commented Nov 9, 2020

start_as_current_span() and start_span() should have identical constructors. For the parameters related to exception and status:

  1. record_exception - whether or not to automatically call record_exception() if exception raised
  2. set_status_on_exception - whether or not to automatically set span status to ERROR if exception raised

As well, from this comment,
adding exception.escaped attribute when exception raised from span being used as a context manager.

@lzchen lzchen requested review from a team, hectorhdzg and owais and removed request for a team November 9, 2020 17:49
Comment on lines +862 to +863
if span._record_exception:
span.record_exception(exc)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it work if on line 854 if you did

with span:
    yield span

to avoid repeating this code?

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 would work. The only reason why I left it in two different places is because record_exception wouldn't know if escaped is True or not.

@lzchen lzchen added the release:required-for-ga To be resolved before GA release label Nov 12, 2020
Copy link
Copy Markdown
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

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

Just a nit.

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

Labels

release:required-for-ga To be resolved before GA release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants