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

Falcon integration closes span early when using streams #10447

Open
lattwood opened this issue Aug 29, 2024 · 2 comments
Open

Falcon integration closes span early when using streams #10447

lattwood opened this issue Aug 29, 2024 · 2 comments
Labels
stale Tracing Distributed Tracing

Comments

@lattwood
Copy link

Hi!

The Falcon web framework can return response content in a traditional manner, ie all at once. Additionally, you can set the stream property on the Response object, and Falcon will use that to generate a response on the fly. When this happens, the span for the http request is closed before the response body is finished streaming- in fact it gets closed before the initial write goes over the wire!

https://github.com/falconry/falcon/blob/e5ada2f958ed4847c3617c34b918b46fca185d73/falcon/response.py#L107-L117

        stream: Either a file-like object with a `read()` method that takes
            an optional size argument and returns a block of bytes, or an
            iterable object, representing response content, and yielding
            blocks as byte strings. Falcon will use *wsgi.file_wrapper*, if
            provided by the WSGI server, in order to efficiently serve
            file-like objects.

            Note:
                If the stream is set to an iterable object that requires
                resource cleanup, it can implement a close() method to do so.
                The close() method will be called upon completion of the request.

This would be great, except Falcon calls all the process_response middleware before it starts to read from the stream.

Middleware call: https://github.com/falconry/falcon/blob/ced5837cda49e9ce7a3437e5479634c27ce98f4e/falcon/app.py#L373-L381

Assembling body call: https://github.com/falconry/falcon/blob/ced5837cda49e9ce7a3437e5479634c27ce98f4e/falcon/app.py#L386

Actual method:
https://github.com/falconry/falcon/blob/ced5837cda49e9ce7a3437e5479634c27ce98f4e/falcon/app.py#L1051-L1099

Now, I'm not entirely sure what's involved in catching exceptions in a response body streamer, but I do know that ddtrace's process_response method should be checking for the existence of a stream on the response method parameter, and if it exists, wrapping it with something that closes the span instead of closing it when stepping back up through the middleware.

Additionally, this might need to be addressed in a different manner in the context of #10446, as Falcon has a completely separate App class for ASGI and WSGI applications- https://github.com/falconry/falcon/blob/ced5837cda49e9ce7a3437e5479634c27ce98f4e/falcon/asgi/app.py#L653-L711

@emmettbutler
Copy link
Collaborator

@lattwood is this fixable against Falcon 3.1.3? I can't totally tell from the description whether this is blocked on a Falcon change.

@lattwood
Copy link
Author

@emmettbutler hmm. so, they should add a middleware hook for when you're returning a stream, but they have the functionality (checking for a close method and invoking it) that would enable accurate span duration.

So I guess-

  • they should add a way to do this properly
  • in the mean time, checking req.stream and wrapping it with something that has a close method should get accurate span durations in the mean time

@mabdinur mabdinur added the Tracing Distributed Tracing label Sep 5, 2024
@github-actions github-actions bot added the stale label Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Tracing Distributed Tracing
Projects
None yet
Development

No branches or pull requests

3 participants