sneako / finch

Elixir HTTP client, focused on performance
MIT License
1.23k stars 114 forks source link

Finch.async_request/3 does not emit all of the expected Telemetry events #229

Closed sneako closed 1 year ago

sneako commented 1 year ago

See https://github.com/sneako/finch/pull/228#issuecomment-1586151203 for additional context

zachallaun commented 1 year ago

Some notes on implementation:

One of the refactorings from #228 was simplifying the %RequestStream{} struct to only contain the attributes required for streaming the request body. That stream is now wrapped in a normal map that the HTTP2 pool hangs on to. This will make it easier to store whatever metadata/etc. we need in this map without having to add more attributes to the RequestStream struct.

To move telemetry inside the pool process, we'll need to start storing metadata and the start times of the telemetry events with these requests. I'm thinking something like:

telemetry_metadata = %{request: req}

request = %{
  stream: RequestStream.new(req.body),
  from: from,
  from_pid: from_pid,
  request_ref: request_ref,
  telemetry: %{
    metadata: telemetry_metadata,
    send: Telemetry.start(:send, telemetry_metadata)
  }
}

Then, when the send completes elsewhere, we can do:

Telemetry.stop(:send, request.telemetry.send, request.telemetry.metadata)

We'll also want to start storing :status and :headers in the metadata when we handle those responses so that they can be emitted when :recv ends.