hyperium / hyper

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

Document that hyper is using tracing #2498

Closed seanmonstar closed 11 months ago

seanmonstar commented 3 years ago

The main lib.rs page would benefit from a section explaining how to view logs in hyper.

nox commented 3 years ago

Would it be best to do that while carrying #2261 to completion?

seanmonstar commented 3 years ago

I think they can happen separately. Really, a larger tracing effort would be good to organize for hyper, as there's several metrics people would like to know, and some simple placement of spans and proper key-value designations in some events in hyper could allow them to more easily get that info.

taqtiqa-mark commented 3 years ago

Really, a larger tracing effort would be good to organize for hyper

OK. Given PR #2261 is not blocking this larger effort, we'll still need some direction on which way @seanmonstar wants to jump.

some simple placement of spans and proper key-value designations in some events in hyper could allow them to more easily get that info

Proposal:

  1. Add #[instrument(level="debug")] to public functions in the Hyper library
  2. Add #[instrument(level="trace")] to Hyper functions that are not public.
  3. Add data via Recording Fields in a subscriber agnostic way on a as-needed basis. (This should address @neoeinstein's observation per issue #2577?)

Context 1:

Issue #2312 has been known for some time. We've hit a reproducible client hang, but can't yet work out if it is the same issue. One reason for this, and we suspect the slow progress on #2312 is related to trace logs.

Context 2:

We hit this issue in the course of trying to debug reproducible Hyper client hang behavior. This is what the existing spans (there are only two, encode_headers and parse_headers) in Hyper look like in Jaeger (via OpenTelemetry)

image

As best we can tell the only proposal to advance Hyper Tracing support is in PR #2261.

We agree with @davidbarsky that (extensive/detailed) OpenTelemetry support can be better handled somewhere else in the stack of: tracing -> tracing-subscriber -> tracing-opentelemetry -> opentelemetry -> opentelemetry-jaeger

However, @neoeinstein does have a point:

Part of opentelemetry includes some semantic attributes that should be added to certain types of spans.

This can likely be address by 3. in the proposal above.

seanmonstar commented 3 years ago

I've opened up #2678 to track overall tracing support in hyper. I'd suggest we keep this one focused on the smaller task of just adding a section to the documentation about using tracing with hyper.

@taqtiqa-mark I don't think we need to instrument all methods in hyper, but rather come up with a few key spans that represent important steps, without requiring the name of the method to be important outside hyper. If you'd like to take a stab a proposing some spans for clients or servers (just an idea), open an issue with a proposal, and we'll link the issue up.

taqtiqa-mark commented 2 years ago

@seanmonstar not sure if the docs here are sufficient to close this?

They'll expand as the implementation roles out, but right now have a 10,00ft view of what tracing in Hyper means (OTel semantic conventions).

seanmonstar commented 11 months ago

The 1.0 docs now mention that tracing is available as an unstable feature. I think that's enough for now.