async-http-client
async-http-client copied to clipboard
swift-server
[DO NOT MERGE] Add tracing instrumentation
Very much WIP.
- Closes slashmo/gsoc-swift-tracing#46
- Closes slashmo/gsoc-swift-tracing#95
FYI, this is how a request currently looks like in my example project:
InstrumentationSystem.bootstrap(FakeTracer())
let client = HTTPClient(eventLoopGroupProvider: .createNew)
var context = BaggageContext()
// Extracted from a prior request
context.fakeTraceID = "123456789"
var request = try HTTPClient.Request(url: "https://httpbin.org/anything", method: .POST, body: .string(#"{"hello":"World"}"#))
request.headers.add(name: "Content-Type", value: "application/json")
let response = try client
.execute(request: request, context: context)
.wait()
In the background, FakeTracer was automatically used to create (and end) a Span, with the following attributes being set:
[
"http.request_content_length": TracingInstrumentation.SpanAttribute.int(17),
"http.status_code": TracingInstrumentation.SpanAttribute.int(200),
"http.target": TracingInstrumentation.SpanAttribute.string("/anything"),
"http.host": TracingInstrumentation.SpanAttribute.string("httpbin.org"),
"http.status_text": TracingInstrumentation.SpanAttribute.string("OK"),
"http.scheme": TracingInstrumentation.SpanAttribute.string("https"),
"http.response_content_length": TracingInstrumentation.SpanAttribute.int(433),
"http.method": TracingInstrumentation.SpanAttribute.string("POST")
]
It also added a trace header to the request containing the fakeTraceID.
@ktoso
do you think its possible and makes sense to create feature/instrumentation integration branch in async-http-client?
do you think its possible and makes sense to create feature/instrumentation integration branch in async-http-client?
Maybe an experimental/tracing branch could be a good idea indeed... so people can more earily try things out as we work on this, and before it actually fully lands hm (esp since it has API impact).
WDYT @Lukasa @artemredkin @tomerd ?
Maybe an experimental/tracing branch could be a good idea indeed... so people can more earily try things out as we work on this, and before it actually fully lands hm (esp since it has API impact).
I'm fine with that. By the way, right now this patch doesn't just have API impact, it's API breaking. I'd like to find a way to avoid that if we can.
I'd like to find a way to avoid that if we can. @Lukasa
We briefly chatted about the reasoning of having it API breaking here: slashmo/gsoc-swift-baggage-context#8
The topic is a bit deeper than "this breaks API so it's bad", it is what is the end goal we want to head towards, and if we agree on this, we can talk more how we get there. This was touched upon in various places but as Tanner just opened up https://forums.swift.org/t/the-context-passing-problem/39162 which also relates to this, perhaps we use that forums thread to discuss this again, and then decide how to roll changes into the client here -- sounds good?
I do not think the end goal should be "context is optional" that causes all kinds of headaches in "where did I drop my trace / context" that are hard to debug (been there, it's a pain), and confirmed by others in: https://forums.swift.org/t/server-distributed-tracing/37464/28?u=ktoso (last replies, and also lessons learnt from how the Go context works and is used IMO); and here where we're in agreement with Johannes that those APIs will change in the future https://github.com/swift-server/async-http-client/pull/227#issuecomment-639989819
Long story short: Yes, context parameter replaces logger parameter, because of the insistence of the "pass a logger" yet context is the actual object that has tracing metadata so so it is the actual thing that needs passing around more than a logger even, but since we also want to pass loggers, we need e.g. a composite context protocol, like designed here: https://github.com/slashmo/gsoc-swift-baggage-context/pull/9 - so we can pass one parameter, and not logger + context all the time. I believe it was very much known up front with Johannes's work and merging the logger parameter that there would have to be an API breaking change once tracing/logging is "done", I talked with Johannes about this at length back then.
As mentioned above, let's first continue in the thread that Tanner kicked off, agree there and loop back here, sounds good?
I added a do-not-merge marker, perhaps that was partially the confusion here as well? This PR is not intended to be merged as is.
Just as a heads up, the main development branch has been changed to main, following the Swift policy on this.
This PR has been re-targeted to main and should just work. However when performing rebases etc please keep this in mind -- you may want to fetch the main branch and rebase onto the main branch from now on since master is not up-to-date anymore and is going to be deleted shortly.