hyperium / hyper

An HTTP library for Rust
https://hyper.rs
MIT License
14.57k stars 1.6k forks source link

RFC Hyper tracing attributes: proc-macro-attributes crate #2742

Closed taqtiqa-mark closed 2 years ago

taqtiqa-mark commented 2 years ago

Updates:

  1. 3-Feb-2022:
    • OpenTelemetry semantic conventions for HTTP spans.
    • Rationalize proposed trace_* attributes.
    • Update examples to current PoC implementation

RFC

Is your feature request related to a problem? Please describe.

Hyper tracing, per issue #2678.

This RFC is concerning the semantics of setting up tracing in a Hyper application.

Specifically, out of scope are the particular values for field(...), skip(...), name and target parameters in the Tracing instrument attribute.

A particular goal is to flexibly compose the tracing #[instrument(...)] attribute

Describe the solution you'd like

Built in tracing support that can be compiled out when not needed - zero runtime penalty when not used. Yet is flexible enough to (hopefully) satisfy the users of Hyper, and the Hyper project itself.

Describe alternatives you've considered

See PR #2697. Having abandoned several aspects such as layers.

Additional context

These Hyper tracing proc-macro-attributes would be behind a cfg_attr guard but that is not relevant right now.

I have a proc-macro crate hyper-tracing-attributes that has the following functionality implemented to the point where I'm confident (code generation) the functionality proposed can be achieved.

The proposal is:

  1. Default use of OpenTelemetry semantic conventions for HTTP spans fields.
  2. A hyperhttp-tracing-attributes crate: I don't anticipate needing consumer/subscriber or layer specific attributes, but if they do end up being needed, one could use e.g. a ~hyper~http-tracing-<layer>-attributes crate etc. In fact there is nothing Hyper specific (yet) about this crate and it could by used to generate Tracing #[instrument(...)] attributes for any HTTP oriented code
    • ~should it be named more generically as http-tracing-attributes crate?~
  3. server_send, etc naming per the finagle standard annotations
  4. attributes to ease composition:
    • MVP: trace_field
    • ~trace_name~
    • Post-MVP: trace_target
    #[server_send(level = ~trace~ Level::INFO, name = "Server::Encode", skip = [dst,msg])]
    #[other]
    #[trace_field(some.thing = tracing::field::Empty)]
    #[trace_field(one.more = "thing")]
    #[another]
    ~#[trace_name("neater")]~
    #[trace_target("bulls-eye")]
    fn encode(msg: Encode<'_, Self::Outgoing>, dst: &mut Vec<u8>) -> crate::Result<Encoder> {
        ...
        if cfg!(feature = "tracing") {
            tracing::Span::current().record("some.thing", "else");
        }
    }

This would generate

    #[other]
    #[another]
    #[instrument( level = "trace",
                    name = "neater",
                    target = "bulls-eye"
                    skip(dst,msg),
                    fields(// Custom
                            some.thing = tracing::field::Empty,
                            one.more   = "thing",
                            // Standardized
                            otel.name           = "Server::Encode",
                            otel.kind           = ?SpanKind::Server,
                            otel.status_code    = ?opentelemetry::trace::StatusCode::Unset,
                            otel.status_message = tracing::field::Empty,
                            // OTel required (HTTP)
                            http.host     = tracing::field::Empty,
                            http.method   = tracing::field::Empty,
                            http.scheme   = tracing::field::Empty,
                            http.target   = tracing::field::Empty,
                            http.url      = tracing::field::Empty,
                            net.peer.ip   = tracing::field::Empty,
                            net.peer.name = tracing::field::Empty,
                            net.peer.port = tracing::field::Empty,
                            // OTel optional (HTTP)
                            http.flavor                 = tracing::field::Empty,
                            http.request_content_length = tracing::field::Empty,
                            // OTel optional (General)
                            net.transport = "IP.TCP",
                        )
                    )
                )]
    fn encode(msg: Encode<'_, Self::Outgoing>, dst: &mut Vec<u8>) -> crate::Result<Encoder> {
        ...
        if cfg!(feature = "tracing") {
            tracing::Span::current().record("some.thing", "else");
        }
    }
taqtiqa-mark commented 2 years ago

Just to be clear, I'd expect that in 80-90% of cases this will suffice:

#[cfg_attr(feature = "tracing", server_send(level = trace, name = "Server::encode", skip = [dst,msg]))]
fn encode(msg: Encode<'_, Self::Outgoing>, dst: &mut Vec<u8>) -> crate::Result<Encoder> {
    ...
}
davidpdrsn commented 2 years ago

Sorry for bringing this up again but whats the advantage of this over a tower middleware like tower-http's Trace? It can be configured to send opentelemetry compatible spans https://github.com/EmbarkStudios/server-framework/blob/main/src/middleware/trace/opentelemetry.rs, without adding anything to hyper.

taqtiqa-mark commented 2 years ago

.... whats the advantage of this over a tower middleware

I'm not sure - possibly none?

However, I don't use tower, nor do I have plans to.

Right now I'm trying to understand/debug some Hyper hanging behavior, also reported independently in currently open Hyper issues. So that is the immediate use case.

taqtiqa-mark commented 2 years ago

Looking, very superficially, at the tower examples: It appears they may capture data from the two spans that are in the current Hyper code base:

  1. parse_headers
  2. encode_headers

If that observation is correct, then this likely complements tower tracing?

taqtiqa-mark commented 2 years ago

Sorry for bringing this up again but whats the advantage of this over a tower middleware .... without adding anything to hyper.

After some clarification via Discord:

The idea here is to instrument the Hyper code itself.

While it is true that much data that passes through Hyper can be 'traced' by a Tower middleware, the purpose here is to trace the Hyper behavior, or more specifically mis-behavior.

The use case is not instrumenting/tracing a 3rd-party app, but rather instrumenting/tracing Hyper itself. For example when trying to debug something such as issue #2312 and similar.

seanmonstar commented 2 years ago

I commented a bit in the pull request about direction.