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.

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

Set status code on ASGI server span #478

Merged
merged 3 commits into from
May 25, 2021
Merged

Set status code on ASGI server span #478

merged 3 commits into from
May 25, 2021

Conversation

jomasti
Copy link
Contributor

@jomasti jomasti commented Apr 28, 2021

Description

Set the status code on the ASGI server span so that the proper response status code is displayed when exported to something like Azure Monitor

Fixes #427

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Updated the tests to verify the status code is in the server span

Does This PR Require a Core Repo Change?

  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

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

@jomasti jomasti marked this pull request as ready for review April 28, 2021 05:24
@jomasti jomasti requested review from a team, owais and hectorhdzg and removed request for a team April 28, 2021 05:24
@@ -225,8 +225,10 @@ async def wrapped_send(message):
if send_span.is_recording():
if message["type"] == "http.response.start":
status_code = message["status"]
set_status_code(span, status_code)
set_status_code(send_span, status_code)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does send here mean sending a response back to the client?
Can the op represented by the outer span have multiple sends or receives?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I know, the callable will be called once per connection, which is one request for HTTP. That should mean only one http.response.start event. Happy to make a change if that is wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe multiple calls to receive and send can occur per HTTP call like from this issue. Should the parent span then reflect ANY error that occurs during the flow? If this is the case, we probably need the response code logic in the receive as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there are multiple send events, but the second one is the response body, and that event does not have another status code.

Copy link
Contributor

Choose a reason for hiding this comment

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

@owais
As discussed in the SIG meeting on 05/13/2021, we were wondering why there is a parent span for the channel in the first place? Would it be possible to remove this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll do a deep dive into ASGI and get back to this. As far as I understand right now, I think the receive span should be the SERVER span and "send" span should be it's child or grandchild. Both of these spans should use links to establish a relation with the channel span. I need some time to dig into this and confirm if my understanding is correct.

@owais
Copy link
Contributor

owais commented May 24, 2021

LGTM. It makes sense for the status code to be on the server span for HTTP requests. We do however need to re-work this instrumentation when it comes to websockets. Ticket to track/discuss it: #505

@codeboten codeboten merged commit b6ed679 into open-telemetry:main May 25, 2021
@jomasti jomasti deleted the fix/asgi-status-code branch June 1, 2021 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FastAPI Instrumentor not populating attributes required for Azure exporter
4 participants