-
Notifications
You must be signed in to change notification settings - Fork 620
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
opentelemetry-instrumentation-asgi: always set status code on duration attributes #2627
opentelemetry-instrumentation-asgi: always set status code on duration attributes #2627
Conversation
@danielhochman You need the CLA signed to get your contribution accepted |
@xrmx Thanks, was waiting on Corp CLA approval. It's signed now! |
…-contrib into middleware-always-set-status-code-on-duration-attrs
@xrmx lint should pass now (tested locally), sorry for the churn. |
@xrmx Thanks again. I think this should be ready to merge! |
...tation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py
Outdated
Show resolved
Hide resolved
...tation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py
Outdated
Show resolved
Hide resolved
@xrmx Thanks for your review! Are we ready to merge? |
Recent changes to asgi instrumentation changes has caused some conflicts. I will resolve them by directly committing to your branch. |
...tation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py
Outdated
Show resolved
Hide resolved
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.
Did som runs in some scenarios I have here and it's working as expected:
For old semconv:
{
"name": "http.server.duration",
"description": "Duration of HTTP server requests.",
"unit": "ms",
"data": {
"data_points": [
{
"attributes": {
"http.scheme": "http",
"http.host": "127.0.0.1:8000",
"net.host.port": 8000,
"http.flavor": "1.1",
"http.method": "GET",
"http.server_name": "localhost:8000",
"http.status_code": 500,
"http.target": "/foobar"
},
}
For new semconv:
{
"name": "http.server.request.duration",
"description": "Duration of HTTP server requests.",
"unit": "s",
"data": {
"data_points": [
{
"attributes": {
"url.scheme": "http",
"network.protocol.version": "1.1",
"http.request.method": "GET",
"http.route": "/foobar",
"http.response.status_code": 500,
"error.type": "500"
}
Description
Bug:
OpenTelemetryMiddleware
records histograms with no status code if the span is non-recording. The decision to include status code on a histogram should not be dependent on tracing decisions.Type of change
How Has This Been Tested?
Does This PR Require a Core Repo Change?
Checklist: