[Tracing] Implement trace header context propagation #862
Conversation
Somewhere along the way of reviewing previous work this bit got lost; This implements context propagation by injecting the apropriate headers from service context into the headers when forming a `Prepared` http request.
| init(_ request: HTTPClientRequest, dnsOverride: [String: String] = [:]) throws { | ||
| init( | ||
| _ request: HTTPClientRequest, | ||
| tracing: HTTPClient.TracingConfiguration? = nil, |
There was a problem hiding this comment.
Nitpick: Shouldn't the non-optional dnsOverride argument go before the optional tracing one?
There was a problem hiding this comment.
Both are defaulted/optional. Could invert if @glbrntt thinks that'd be better tho hmm
There was a problem hiding this comment.
Isn't dnsOverride a non-optional dictionary defaulting to empty? Again, very nitpick and feel free to ignore the comment.
There was a problem hiding this comment.
It's internal API so I don't mind either way, and neither requires the caller to set it explicitly so the order doesn't matter.
However, I would've put dnsOverride before tracing as the dnsOverride is used for "core" functionality (i.e. actually making the request) vs. "extra" functionality (observability).
| //===----------------------------------------------------------------------===// | ||
|
|
||
| @_spi(Tracing) import AsyncHTTPClient // NOT @testable - tests that need @testable go into HTTPClientInternalTests.swift | ||
| @_spi(Tracing) @testable import AsyncHTTPClient |
There was a problem hiding this comment.
The previous comment was there for a reason, so that internal API isn't accidentally used in these tests. Can you move the new test?
There was a problem hiding this comment.
Okey, should I perhaps make HTTPClientTracingInternalTests.swift then?
There was a problem hiding this comment.
Yeah, that seems like the right place
Somewhere along the way of reviewing previous work this bit got lost;
This implements context propagation by injecting the apropriate headers from service context into the headers when forming a
Preparedhttp request.