feat(tower): instrument outgoing HTTP requests with a client layer - #700
Open
jan-xyz wants to merge 8 commits into
Open
feat(tower): instrument outgoing HTTP requests with a client layer#700jan-xyz wants to merge 8 commits into
jan-xyz wants to merge 8 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #700 +/- ##
=======================================
- Coverage 79.7% 72.2% -7.6%
=======================================
Files 133 135 +2
Lines 28369 28391 +22
=======================================
- Hits 22623 20505 -2118
- Misses 5746 7886 +2140
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Member
|
@francoposa - wondering if you would be the right person to review this PR ? |
Contributor
|
Could we do a clean refactor PR first to apply the namespacing to the existing code, then add the client code and all the benchmarking as a separate PR after? I think the refactor may have a bit of discussion but be relatively quick to get through, while the client code could take a bit longer. |
The crate exposed a single flat set of names (HTTPLayer, HTTPLayerBuilder, the route and attribute extractors) that all implicitly meant "HTTP server". That leaves no room for the other protocols and directions the crate is expected to cover, and readers cannot tell from a name whether it applies to inbound or outbound traffic. Group the middleware into `http::server`, move the pluggable extractors into `http::extractors`, and pull the shared attribute helpers into a private `common` module, so each name states which protocol and which side of the connection it instruments. This is a rename and file move only; the emitted spans, metrics, and attributes are unchanged.
Every attribute and metric name was re-exported through a local `*_LABEL` or `*_METRIC` alias whose only job was to point at the corresponding semantic-conventions constant. The indirection meant a reader had to jump to a second location to learn which convention a label actually maps to, and it made the same name appear under two spellings across the crate. Use the `opentelemetry-semantic-conventions` constants at the point of use and delete the aliases. The constants resolve at compile time, so this is free at runtime, and the emitted names are unchanged.
The unreleased notes still referred to type names that no longer exist, so anyone upgrading would look for symbols the crate does not export. Rewrite the unreleased entry around the new module paths and give the migration guide a direct before/after mapping from the old flat names.
4 tasks
jan-xyz
force-pushed
the
tower-http-client
branch
from
July 30, 2026 17:21
50e588a to
315bcfc
Compare
jan-xyz
changed the title
Moving the shared attribute helpers into their own module put them in a different codegen unit from their callers, and without an inline hint the compiler stopped inlining them into the per-request path. That showed up as roughly 3% added overhead on every instrumented request, even when both the tracer and meter are no-ops. Mark the helpers inline so the move stays free. Measured against the previous release point, this brings the no-op request path back to parity; the remaining run-to-run variation is larger than the difference.
jan-xyz
force-pushed
the
tower-http-client
branch
from
July 30, 2026 17:56
315bcfc to
2c65a7e
Compare
Only inbound traffic was instrumented, so a service using this crate produced server spans but nothing for the calls it made itself. Traces therefore ended at each service boundary and the downstream hop had no parent to attach to, which breaks distributed traces across services. Add a client layer that emits a client span and the standard `http.client.*` metrics per outgoing request and injects the current trace context into the request headers, so the callee can continue the trace. Tracing and metrics are each toggleable per layer for callers who only want one signal, and the two layers are now behind separate Cargo features so a server-only or client-only build does not compile code it will never call. Unlike the server layer the client defaults to method-only span names: an outbound URL has no route template to normalize against, and using the raw path would push request-specific values into span names and metric labels.
The server layer had a benchmark to catch performance regressions but the new client layer had none, so any future change to its hot path could slow every outgoing request unnoticed. Mirror the server benchmark for the client: call a stub transport that returns immediately, so the measurement reflects span creation, attribute extraction, header injection, and metric recording rather than the network. Both benchmarks run with the default no-op propagator, keeping the numbers about the layer itself instead of a particular propagator's header encoding.
jan-xyz
force-pushed
the
tower-http-client
branch
from
July 30, 2026 18:00
2c65a7e to
8e97251
Compare
Member
Author
|
@francoposa good idea, I created a separate PR (#717) for moving the server to the new structure, let's discuss that one first, and once it's merged we can come back to this one. |
# Conflicts: # opentelemetry-instrumentation-tower/CHANGELOG.md
# Conflicts: # opentelemetry-instrumentation-tower/CHANGELOG.md
Contributor
|
Previous comment deleted, a rogue PR review agent decided to ignore all its restrictions |
Contributor
|
I would like there to be a client code example added as part of this - it could even supplant the k6 load-gen script in the docker compose setup. IMO think it's especially important as the "Tower Service as client middleware" is not as obvious or well-trodden of a path in other library documentation. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #
Design discussion issue (if applicable) #
Changes
Stacked on #717 — the first three commits here are that PR. Review #717 first; this description covers only the two commits on top of it. GitHub will not let a fork branch be used as a PR base, so this cannot literally target #717; the diff collapses to client-only once #717 merges.
Only inbound traffic was instrumented, so a service using this crate produced server spans but nothing for the calls it made itself. Traces therefore ended at every service boundary: the downstream hop had no parent context to attach to, and the resulting traces were per-service islands rather than one distributed trace. The client layer closes that gap by emitting a client span and the standard
http.client.*metrics per outgoing request, and injecting the current trace context into the outgoing headers so the callee can continue the trace.Two deliberate differences from the server layer are worth calling out. The client defaults to method-only span names, because an outbound URL carries no route template to normalize against and using the raw path would push request-specific values into span names and metric labels. And the two layers now sit behind separate Cargo features, so a server-only or client-only deployment does not compile instrumentation it will never call. Tracing and metrics are also individually toggleable per layer for callers who only collect one signal.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes