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

Jump to bottom

Record exception on context manager exit #1162

Merged
merged 1 commit into from
Oct 7, 2020
Merged

Record exception on context manager exit #1162

merged 1 commit into from
Oct 7, 2020

Conversation

owais
Copy link
Contributor

@owais owais commented Sep 25, 2020 •
edited
Loading

Description

This updates the tracer context manager to record exceptions automatically on
exit if an exception was raised within the context manager's context.

This should be helpful in some situations when manually instrumenting apps. For example,
it can help record any uncaught/unexpected exceptions which one would expect from a tracing
system. This can also be helpful in simplifying auto-instrumentations. I think this should be the
default behavior but happy to hear other thoughts. Otel spec does not say anything about this and leaves span creation/activation to each language implementation. Both OpenTracing and OpenCensus did it AFAICT:

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • Unit Test

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@owais owais changed the title Record exception on context manager exit DO NOT REVIEW: Record exception on context manager exit Sep 25, 2020
@owais owais force-pushed the record-exceptions branch 2 times, most recently from c343f53 to 75e46c9 Compare September 25, 2020 02:20
@owais owais changed the title DO NOT REVIEW: Record exception on context manager exit Record exception on context manager exit Sep 25, 2020
@owais owais marked this pull request as ready for review September 25, 2020 02:31
@owais owais requested a review from a team September 25, 2020 02:31
@owais owais force-pushed the record-exceptions branch 9 times, most recently from ccac4bb to 5c04297 Compare September 25, 2020 03:45
@lzchen lzchen self-assigned this Oct 1, 2020
@owais owais force-pushed the record-exceptions branch from 694b26c to f5abcd5 Compare October 2, 2020 15:13
toumorokoshi
toumorokoshi suggested changes Oct 3, 2020
View reviewed changes
Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

The goal looks great! +1 to add this convenience.

I think we should re-examine adding that field to the span though. See my comment above.

opentelemetry-api/src/opentelemetry/trace/__init__.py Outdated Show resolved Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py Outdated Show resolved Hide resolved
@owais owais force-pushed the record-exceptions branch 10 times, most recently from 29f7ef6 to 2222d26 Compare October 4, 2020 01:45
toumorokoshi
toumorokoshi suggested changes Oct 5, 2020
View reviewed changes
Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

One minor question (incongruent default value from "record_exception") but looking really good!

...pentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py Show resolved Hide resolved
opentelemetry-api/src/opentelemetry/trace/__init__.py Outdated Show resolved Hide resolved
opentelemetry-sdk/CHANGELOG.md Outdated Show resolved Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py Outdated Show resolved Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py Outdated Show resolved Hide resolved
@owais owais force-pushed the record-exceptions branch 3 times, most recently from 5a72598 to 7e30859 Compare October 5, 2020 20:15
@owais owais force-pushed the record-exceptions branch from 7e30859 to cf94b05 Compare October 7, 2020 02:15
@owais
Record exception on context manager exit
57a01cd 
This updates the tracer context manager to automatically record
exceptions as events on exit if an exception was raised within the
context manager's context.
@owais owais force-pushed the record-exceptions branch from cf94b05 to 57a01cd Compare October 7, 2020 02:18
toumorokoshi
toumorokoshi approved these changes Oct 7, 2020
View reviewed changes
Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@lzchen lzchen merged commit 803f582 into open-telemetry:master Oct 7, 2020
@owais owais deleted the record-exceptions branch October 7, 2020 19:03
@lzchen lzchen mentioned this pull request Nov 23, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lzchen lzchen lzchen approved these changes

@toumorokoshi toumorokoshi toumorokoshi approved these changes

Assignees

@lzchen lzchen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@owais @toumorokoshi @lzchen