logfire icon indicating copy to clipboard operation
logfire copied to clipboard

Allow HTTPX instrumentation to capture more request info, and improve API docs

Open samuelcolvin opened this issue 11 months ago • 8 comments

Description

We should add the following kwargs to instrument_httpx:

  • capture_request_headers: bool | None = None
  • capture_request_body: bool | None = None
  • capture_response_headers: bool | None = None
  • capture_response_body: bool | None = None
  • capture_all: bool = False - just sets all the above to True

We can raise an error if you use these together with async_request_hook etc.

We can add a note saying that capturing the body in production might increase the amount of data collected significantly.

Also HTTPXInstrumentKwargs is not included in docs which makes it basically undocumented.

We should probably do the same for requests.

samuelcolvin avatar Dec 09 '24 15:12 samuelcolvin

@Kludex is currently working on headers in https://github.com/pydantic/logfire/pull/671 which is close to finishing.

After that comes capturing JSON bodies which @Kludex will also work on, I've provided him with a PoC implementation in slack.

Then https://github.com/pydantic/logfire/pull/668 can go through.

We discussed eventually capturing other types of bodies:

  1. Forms, which could be done by walking up the stack to retrieve the data from a frame instead of parsing bytes
  2. Arbitrary non-streamed bodies
  3. Arbitrary streamed bodies

I briefly toyed with the last case, leaving the code here in case we want it:

from dataclasses import dataclass
from typing import Iterable
import httpx
from httpx._content import IteratorByteStream
import logfire
from logfire.propagate import attach_context, get_context, ContextCarrier
from opentelemetry.trace import get_current_span, Span


def content():
    yield b"Hello, "
    yield b"world!"


logfire.configure()


@dataclass
class LoggingStream:
    stream: Iterable[bytes]
    ctx: ContextCarrier
    current_span: Span

    def __iter__(self):
        result = []
        for chunk in self.stream:
            result.append(chunk)
            yield chunk
        body = b"".join(result)
        if self.current_span.is_recording():
            # TODO this requires the bytes to be decodable as text
            self.current_span.set_attribute("http.body", body)
        else:
            with attach_context(self.ctx):
                logfire.info("Streamed body", body=body)


def request_hook(span, info):
    record_body(info)


def record_body(info):
    if isinstance(info.stream, IteratorByteStream):
        info.stream._stream = LoggingStream(
            info.stream._stream, get_context(), get_current_span()
        )


logfire.instrument_httpx(request_hook=request_hook)

r = httpx.post("https://httpbin.org/post", content=content())
print(r.text)

alexmojaki avatar Dec 16 '24 16:12 alexmojaki

What else are we missing to close this? I can work on it today.

Kludex avatar Dec 24 '24 09:12 Kludex

  1. capture_response_text_body
  2. Documentation of all the capture_* args
  3. capture_all
  4. Replacing Unpack[HTTPXInstrumentKwargs]
  5. Optionally refactoring response info similar to https://github.com/pydantic/logfire/pull/722
  6. Optionally moving the enhanced request/response info classes into the public logfire.integrations.httpx module and documenting them with example hooks, the simplest being how to capture only request or response headers with a single method call in a hook.

alexmojaki avatar Dec 24 '24 10:12 alexmojaki

@Kludex can you do 2-4?

alexmojaki avatar Dec 30 '24 14:12 alexmojaki

You working on 3? If so, I can work on 2-4 after.

Kludex avatar Dec 30 '24 14:12 Kludex

I am not.

alexmojaki avatar Dec 30 '24 14:12 alexmojaki

I am not.

I don't understand if you are going to, if you think it shouldn't be added, or if you are implying that I can and should work on it.

Kludex avatar Dec 30 '24 14:12 Kludex

Waiting review:

  • https://github.com/pydantic/logfire/pull/753
  • https://github.com/pydantic/logfire/pull/751

I'll document tomorrow.

Kludex avatar Dec 30 '24 16:12 Kludex