DataDog / dd-trace-py

Datadog Python APM Client
https://ddtrace.readthedocs.io/
Other
532 stars 408 forks source link

Falcon integration closes span early when using streams #10447

Open lattwood opened 2 weeks ago

lattwood commented 2 weeks ago

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 commented 2 weeks ago

@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 commented 2 weeks ago

@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-