-
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
Tornado instrumetnation status_code 4ever 500 if error exsist #997
Comments
@nicholasgribanov It was unclear from the description what is the problem here? Can you explain what is the issue and how does the proposed solution fixes the issue? |
@srikanthccv thank's for a fast answer.
I wait spans only with http-status more when 500. But, now, it doesn't work. Beacause, on the Tornado instrumentation level, server spans with error and http-status not equal 404, will forcibly setting http.status_code to 500 https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/instrumentation/opentelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/__init__.py#L372-L379. It's not right behavior. If you are waiting spans onlyf with http-status more than 500, you must not getting spans with 400 error. Lines 62 to 63 in dbb35a2
|
Ok, I see there is a discrepancy. I actually wonder why do we change the status code from 4XX to 500, @owais, do you have any input? |
Any updates ? |
Here https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/instrumentation/opentelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/__init__.py#L372-L379, when span finished, status_code is setting to 500, if error exists and status code from handler not equals 404.
Is it correct behavior ?
I see, it needed for setting description in Status. But, in this method, if status_code <=499, Status must be set to UNSET. In our case, if status_code <=499 will set ERROR.
opentelemetry-python-contrib/opentelemetry-instrumentation/src/opentelemetry/instrumentation/utils.py
Lines 62 to 63 in dbb35a2
Maybe needed extend this condition https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/instrumentation/opentelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/__init__.py#L372 to all 4xx http codes instead of just 404?
The text was updated successfully, but these errors were encountered: