francoposa / tower-otel-http-metrics

OpenTelemetry HTTP Metrics Middleware Layer for Tower-Compatible Servers (Axum, Hyper)
https://docs.rs/tower-otel-http-metrics/latest/tower_otel_http_metrics
MIT License
8 stars 5 forks source link

Other instruments #10

Open hardbyte opened 1 month ago

hardbyte commented 1 month ago

Hey great work with the crate!

I forked a similar repo that was tied to prometheus to make it use the opentelemetry SDK. Perhaps we could combine efforts?

I think some of these instruments would make sense in this library

francoposa commented 1 month ago

Hi, I definitely welcome some contribution!

Just regarding my sort of my desired scope for the crate at the moment:

I want to keep scope for this library strictly to the OTEL HTTP server spec.

Meaning w.r.t the instruments you linked:

I would want to keep the labels as strict to OTEL spec as possible as well. I think I personally would aim to take a bit stricter approach to the url.scheme label, make server.address and server-port opt-in, etc.

hardbyte commented 1 week ago

Great, all sounds sensible to me. I've opened #11 for the first one.

For request size - do you think trusting the CONTENT_LENGTH header is the right approach to avoid consuming the body?

francoposa commented 1 week ago

For request size - do you think trusting the CONTENT_LENGTH header is the right approach to avoid consuming the body?

I think this is the right approach for a metrics middleware.

Double-checking the body instead of trusting the CONTENT_LENGTH header is more of an approach for debugging a specific issue with a misbehaving client.

Given that the spec suggests to look for it in the header and that it's optional, so I think the approach is to just set it if it's in the header.

[1]: The size of the request payload body in bytes. This is the number of bytes transferred excluding headers and is often, but not always, present as the Content-Length header. For requests using transport encoding, this should be the compressed size.

As a reference point, the tower middleware which is specifically for limiting the body size also trusts the header: https://docs.rs/tower-http/latest/src/tower_http/limit/service.rs.html

francoposa commented 2 days ago

It appears we just need server.response.body.size to consider this accomplished. There's already a separate issue for further header parsing attempts to set the url.scheme label.