-
Notifications
You must be signed in to change notification settings - Fork 599
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.
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
Fix Tornado errors mapping to 500 #1048
Conversation
6dbb031
to
ea8967c
Compare
@@ -369,14 +369,15 @@ def _finish_span(tracer, handler, error=None): | |||
status_code = error.status_code | |||
if not ctx and status_code == 404: | |||
ctx = _start_span(tracer, handler, _time_ns()) | |||
if status_code != 404: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@owais
Was there a reason for explicitly checking for non-404s previously?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I undertood right, it was needed for not setting traceback if status equals 404, in other case do it. I've changed this behavior, and now traceback will be setted only for server errors - 500s
instrumentation/opentelemetry-instrumentation-tornado/tests/tornado_test_app.py
Outdated
Show resolved
Hide resolved
instrumentation/opentelemetry-instrumentation-tornado/tests/test_instrumentation.py
Outdated
Show resolved
Hide resolved
0f7d1be
to
1188986
Compare
@nicholasgribanov |
Head branch was pushed to by a user without write access
1188986
to
d626e37
Compare
|
d626e37
to
dca157f
Compare
@@ -6,7 +6,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), | |||
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). | |||
|
|||
## [Unreleased](https://github.com/open-telemetry/opentelemetry-python/compare/v1.10.0-0.29b0...HEAD) | |||
|
|||
- `opentelemetry-instrumentation-tornado` Fix Tornado errors mapping to 500 | |||
([#1048])(https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1048) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this to Fixed
section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -6,7 +6,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), | |||
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). | |||
|
|||
## [Unreleased](https://github.com/open-telemetry/opentelemetry-python/compare/v1.10.0-0.29b0...HEAD) | |||
|
|||
- `opentelemetry-instrumentation-tornado` Fix Tornado errors mapping to 500 | |||
([#1048])(https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1048) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this to Fixed
section.
dca157f
to
87cfa62
Compare
Description
Fix bug in Tornado instrumentation. Current implementation erase http status codes and set 500 for any types of exceptions (tornado.web.HTTPError and others). But in Tornado implemantation setting http status 500 if exception not tornado.web.HTTPError. Otherwise, http status will set from exception. This fix repeat Tornado implementation logic.
Fixes #997
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.