Skip to content

Conversation

ktoso
Copy link
Contributor

@ktoso ktoso commented Sep 10, 2025

Almost 5 years after the initial tracing proposal PR for this HTTPClient, I'd like to propose it again.

Popular server packages should be embracing distributed tracing, and none is more crucial in this than the async-http-client.

This PR proposes an approach to include tracing with:

  • using package traits, from Swift 6.1
  • the TracingSupport trait is enabled by default
  • the trait can be disabled to entirely remove the tracing dependency along with any code using it (and potential overheads).

The details for where to start/end spans are TBD, we can do better than this.

Currently there is a single span that starts before we send the request, and ends as we get the response head. We can further improve this with spans for streaming the request body, spans for the time it takes to send the request etc. However first I'd like to get agreement on this approach for the instrumentation.

The spans

This creates very basic spans. Basically when you do client.get()/post() etc, as well as the asynchronous try await client.execute() we create a span that:

  • starts: when the request starts -- we could keep improving where EXACTLY this starts, there's lots of opinions to have about this 😉
  • ends: when we receive the response head

There's no spans yet for the response body streaming or the waiting in the pool etc. All this can be added incrementally.

⏭️ The spans don't have fancy attributes yet;

⏭️ and the error cases are likely not fully covered. If someone more familiar with HTTPClient wants to take that on that'd be best, but I can follow-up with more handling once we're happy with this approach in general.

I think this should be done in follow-ups, so we can keep polishing up the tracing support as we use it in practice.

Availability woes

It's been somewhat painful to not cause the entire HTTPClient types to suddenly get @Availability which would be a breaking change; So I'm playing a bunch of tricks with storing any Sendable as storage and casting that... We could probably use unsafe bitcasts, but taking this conservatively for now -- it should work first and foremost, and we can spend time shaving off overhead eventually.

I don't like one bit where we're doing that, the mutating func startRequestSpan<T>(tracer: T?) {} is kinda horrible, but if we make this have Tracer we end up forcing the surrounding type to have availability...

⏭️ We could think of some better ways to express this, but it's all internal so I think we can take this incrementally.

Configuration

It is possible to set a tracer "per client" by using the Configuration, which in combination with the in memory tracer (Incoming here apple/swift-distributed-tracing#180), will be able to be used for testing. One can also pass nil to configuration to disable picking up the global tracer during configuration.

We can use this for testing, and it's important for e.g. the otel library so when exporting while using the httpclient, we don't necessarily also cause spans of that to be exported -- if we don't care we can disable it this way. (alternatively, by making a parent span inside swift-otel that is NOT recording also works)

Context propagation

✅ This also includes header propagation, although I've ran out of night to make a test for it -- I'll follow up on it shortly.

Resolves slashmo/gsoc-swift-tracing#46
Resolves slashmo/gsoc-swift-tracing#95

Replaces #320
Replaces #289

cc @slashmo @simonjbeaumont @czechboy0 @FranzBusch @fabianfett

Copy link

@czechboy0 czechboy0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach looks good assuming we really need the trait. I'd also be totally open to this being added without a trait, like logging - curious what others think. From my read a lot of the complexity in this PR is caused by that trait, so removing it would simplify.

Also regarding where the tracing happens, the v1 of this could just be a structured span inside execute, maybe?

I assume folks would be okay only getting spans when using the async/await API, and not the pre-concurrency one.

@czechboy0
Copy link

Actually, in case folks would like to see the super naive way to do tracing in AHC, if having the trait is not a requirement, here's what it could look like: #858

@ktoso
Copy link
Contributor Author

ktoso commented Sep 11, 2025

I'm ok with making the simplest possible span at first here, we can do that at first.

I also am trying to land the I'm memory tracer so this pr would not have to have a copy paste one anymore

@ktoso
Copy link
Contributor Author

ktoso commented Sep 12, 2025

I pushed this along some more, cleaned up and added some tests that cover async execute, the ELF based get/post. I did have to dance around the availability problems a bit... Perhaps there's some cleaner ways to pull it off...

This is nearing something that might be acceptable, please review folks.

@ktoso ktoso marked this pull request as ready for review September 12, 2025 07:20
@ktoso ktoso requested review from weissi, Lukasa and glbrntt September 12, 2025 10:16
@ktoso
Copy link
Contributor Author

ktoso commented Sep 12, 2025

Seems we may have overall agreement on the approach, so I'm going to add some more test cases to make sure we're handling errors and cancellations well -- and ping again once this is more complete :-)

@ktoso ktoso requested review from Lukasa and FranzBusch September 17, 2025 09:03
Copy link
Collaborator

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo one nit

@ktoso
Copy link
Contributor Author

ktoso commented Sep 24, 2025

FYI; Hmm, that failure is a warning/as-error in a file this PR didn't touch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Record HTTP requests made by AsyncHTTPClient Make BaggageContext part of AsyncHTTPClient API
9 participants