Skip to content
This repository was archived by the owner on Apr 23, 2021. It is now read-only.

Make BaggageContext part of AsyncHTTPClient API #46

Open
slashmo opened this issue Jun 22, 2020 · 6 comments · May be fixed by swift-server/async-http-client#289
Open

Make BaggageContext part of AsyncHTTPClient API #46

slashmo opened this issue Jun 22, 2020 · 6 comments · May be fixed by swift-server/async-http-client#289
Assignees
Labels
i:http-client Relates to async-http-client integration
Milestone

Comments

@slashmo
Copy link
Owner

slashmo commented Jun 22, 2020

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

@slashmo slashmo added the i:http-client Relates to async-http-client integration label Jun 22, 2020
@ktoso
Copy link
Collaborator

ktoso commented Jul 22, 2020

Maybe a good time to do this actually

@pokryfka
Copy link
Contributor

@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 #72 (comment) and #48

@ktoso
Copy link
Collaborator

ktoso commented Jul 22, 2020

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
Copy link
Contributor

pokryfka commented Jul 22, 2020

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 #72 (comment) and let me know if its not clear

@ktoso
Copy link
Collaborator

ktoso commented Jul 22, 2020

👍 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
Copy link
Contributor

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 #95

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
i:http-client Relates to async-http-client integration
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants