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

Re-raise errors caught in opentelemetry.sdk.trace.Tracer.use_span() #469

Merged
merged 1 commit into from
Mar 9, 2020
Merged

Re-raise errors caught in opentelemetry.sdk.trace.Tracer.use_span() #469

merged 1 commit into from
Mar 9, 2020

Conversation

ThePumpingLemma
Copy link
Contributor

I was expecting the error to get propagated but instead it was swallowed by the context manager.

@ThePumpingLemma ThePumpingLemma requested a review from a team March 7, 2020 01:13
@codecov-io
Copy link

codecov-io commented Mar 7, 2020 •
edited
Loading

Codecov Report

Merging #469 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #469      +/-   ##
==========================================
+ Coverage   88.66%   88.71%   +0.04%     
==========================================
  Files          43       43              
  Lines        2206     2206              
  Branches      249      249              
==========================================
+ Hits         1956     1957       +1     
  Misses        176      176              
+ Partials       74       73       -1
Impacted Files Coverage Δ
...emetry-sdk/src/opentelemetry/sdk/trace/__init__.py 92.46% <100%> (+0.39%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ed98eb...ff587a8. Read the comment docs.

@ThePumpingLemma
Copy link
Contributor Author

I signed it

@mauriciovasquezbernal
Copy link
Member

mauriciovasquezbernal commented Mar 7, 2020 •
edited
Loading

The fix looks good, thanks a lot!
Would you mind implementing a test so we are sure this doesn't get broken again?

@mauriciovasquezbernal mauriciovasquezbernal added tracing sdk Affects the SDK package. labels Mar 7, 2020
@ThePumpingLemma
Copy link
Contributor Author

The fix look good, thanks a lot!
Would you mind implementing a test so we are sure this doesn't get broken again?

Sure, I added a test that checks if the error is propagated when a status is explicitly set.

mauriciovasquezbernal
mauriciovasquezbernal approved these changes Mar 9, 2020
View reviewed changes
Copy link
Member

@mauriciovasquezbernal mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

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

LGTM, thanks again!

toumorokoshi
toumorokoshi approved these changes Mar 9, 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.

Nice catch , thanks!

@toumorokoshi toumorokoshi merged commit 1f1e699 into open-telemetry:master Mar 9, 2020
toumorokoshi added a commit to toumorokoshi/opentelemetry-python that referenced this pull request Mar 16, 2020
@toumorokoshi
Adding changelog entries for the 0.5b0 release
664b86d 
Total Changelog:

Documentations has been significantly overhauled, including:

* a getting started guide
* examples has been consolidated to an docs/examples folder
* several minor improvements to the examples in each extension's readme.

- Adding Correlation Context API and propagator
  ([open-telemetry#471](open-telemetry#471))
- Adding a global configuration module to simplify setting and getting
  globals
  ([open-telemetry#466](open-telemetry#466))
- Rename metric handle to bound metric
  ([open-telemetry#470](open-telemetry#470))
- Moving resources to sdk
  ([open-telemetry#464](open-telemetry#464))
- Implementing propagators to API to use context
  ([open-telemetry#446](open-telemetry#446))
- Implement observer instrument for metrics
  ([open-telemetry#425](open-telemetry#425))
- Adding named meters, removing batchers
  ([open-telemetry#431](open-telemetry#431))
- Renaming TraceOptions to TraceFlags
  ([open-telemetry#450](open-telemetry#450))
- Renaming TracerSource to TraceProvider
  ([open-telemetry#441](open-telemetry#441))

- Adding Correlation Context SDK and propagator
  ([open-telemetry#471](open-telemetry#471))
- Adding OT Collector metrics exporter
  ([open-telemetry#454](open-telemetry#454))
- Improve validation of attributes
  ([open-telemetry#460](open-telemetry#460))
- Re-raise errors caught in opentelemetry.sdk.trace.Tracer.use_span()
  (open-telemetry#469)
  ([open-telemetry#469](open-telemetry#469))
- Adding named meters, removing batchers
  ([open-telemetry#431](open-telemetry#431))
- Make counter and MinMaxSumCount aggregators thread safe
  ([open-telemetry#439](open-telemetry#439))

- Initial release. Support is included for both trace and metrics.
@toumorokoshi toumorokoshi mentioned this pull request Mar 16, 2020
Merged
c24t pushed a commit to c24t/opentelemetry-python that referenced this pull request Mar 16, 2020
@toumorokoshi @c24t
Adding changelog entries for the 0.5b0 release
8004571 
Total Changelog:

Documentations has been significantly overhauled, including:

* a getting started guide
* examples has been consolidated to an docs/examples folder
* several minor improvements to the examples in each extension's readme.

- Adding Correlation Context API and propagator
  ([open-telemetry#471](open-telemetry#471))
- Adding a global configuration module to simplify setting and getting
  globals
  ([open-telemetry#466](open-telemetry#466))
- Rename metric handle to bound metric
  ([open-telemetry#470](open-telemetry#470))
- Moving resources to sdk
  ([open-telemetry#464](open-telemetry#464))
- Implementing propagators to API to use context
  ([open-telemetry#446](open-telemetry#446))
- Implement observer instrument for metrics
  ([open-telemetry#425](open-telemetry#425))
- Adding named meters, removing batchers
  ([open-telemetry#431](open-telemetry#431))
- Renaming TraceOptions to TraceFlags
  ([open-telemetry#450](open-telemetry#450))
- Renaming TracerSource to TraceProvider
  ([open-telemetry#441](open-telemetry#441))

- Adding Correlation Context SDK and propagator
  ([open-telemetry#471](open-telemetry#471))
- Adding OT Collector metrics exporter
  ([open-telemetry#454](open-telemetry#454))
- Improve validation of attributes
  ([open-telemetry#460](open-telemetry#460))
- Re-raise errors caught in opentelemetry.sdk.trace.Tracer.use_span()
  (open-telemetry#469)
  ([open-telemetry#469](open-telemetry#469))
- Adding named meters, removing batchers
  ([open-telemetry#431](open-telemetry#431))
- Make counter and MinMaxSumCount aggregators thread safe
  ([open-telemetry#439](open-telemetry#439))

- Initial release. Support is included for both trace and metrics.
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
@obecny @mayurkale22
chore: fixing problem with load event and performance for loadend (op…
d5141f7 
…en-telemetry#469)

* chore: fixing problem with load event and performance for loadend

* chore: clean up in unit tests

* chore: removing delay from timeouts

* chore: removing delay from timeout
@ThePumpingLemma ThePumpingLemma deleted the reraise-error-in-use-span branch January 8, 2023 02:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mauriciovasquezbernal mauriciovasquezbernal mauriciovasquezbernal approved these changes

@toumorokoshi toumorokoshi toumorokoshi approved these changes

Assignees
No one assigned
Labels
sdk Affects the SDK package. tracing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ThePumpingLemma @codecov-io @mauriciovasquezbernal @toumorokoshi