search

slashmo / gsoc-swift-tracing

WIP collection of Swift libraries enabling Tracing on Swift (Server) systems.
https://summerofcode.withgoogle.com/projects/#6092707967008768
Apache License 2.0
20 stars 1 forks source link

Make BaggageContext part of AsyncHTTPClient API #46

Open slashmo opened 4 years ago

slashmo commented 4 years ago

https://github.com/slashmo/gsoc-swift-tracing/pull/45/files#r443569191

ktoso commented 4 years ago

Maybe a good time to do this actually

pokryfka commented 4 years ago

@ktoso

Maybe a good time to do this actually

On top of that, I fail to see how existing TracingInstrument could be used to instrument downstream HTTP APIs without "currentSpan"

in "proof of a concept" XRayMiddleware "currentSpan" equivalent is used; but its a source of problems and I'd be very happy to get rid of that; also as far as Lambda invocations are concerned the context and "currentSpan" are explicitly set at the beginning of each lambda invocation - which kind of works;

see also https://github.com/slashmo/gsoc-swift-tracing/issues/72#issuecomment-660840427 and https://github.com/slashmo/gsoc-swift-tracing/issues/48

ktoso commented 4 years ago

On top of that, I fail to see how existing TracingInstrument could be used to instrument downstream HTTP APIs without "currentSpan"

"In Baggage we trust" is the motto 😉

The instrument should perform inject/extract based on baggage context;

While in a trace, and making an http request people will: http.get(url, context: trace.context) which is the baggage context of the span; it should contain everything you'll need to perform the instrument.inject(context:into:...) once you're (by the instrumented http client) eventually as it processes the request before it sends it out.

pokryfka commented 4 years ago

http.get(url, context: trace.context)

👍 thats great, it will however require changing (extending) of the HTTP client API so that user needs to pass the context (of the current span) explicitly;

still it does not solve how it to get HTTP response status as defined by XRay (arguably its not a critical feature but "nice to have") please see https://github.com/slashmo/gsoc-swift-tracing/issues/72#issuecomment-660840427 and let me know if its not clear

ktoso commented 4 years ago

👍 thats great, it will however require changing (extending) of the HTTP client API so that user needs to pass the context (of the current span) explicitly;

Indeed, and that's what we'll do.

I'll look into your other question soon, I need to dig into what the core issue is.

pokryfka commented 4 years ago

I'll look into your other question soon, I need to dig into what the core issue is.

I tried to address the issue and made a proof of concept implementation, see https://github.com/slashmo/gsoc-swift-tracing/issues/95