Conversation
| F: MonadThrow[F], | ||
| backend: Http4sHttpBackend[F], | ||
| logger: Logger[F] | ||
| def of[F[_]: {MonadThrow as F, Http4sHttpBackend as B, Logger as L}, S]( |
There was a problem hiding this comment.
F and L are pretty standard notation. But B isn't and backend is much more descriptive as a var name.
| mergedExt = Some(traceExt).filterNot(_.isEmpty) | ||
| result <- poll( | ||
| wrapped | ||
| .subscribeInternal[D](document, operationName, variables, mergedExt) |
There was a problem hiding this comment.
Not much, but we are dupicating some logic here. Maybe we can invoke requestInternal and pass extensions through it.
There was a problem hiding this comment.
Yeah, there is no harm in passing them on the requests, though they are wasted as they are not read since we have the traceparent on the headers. otoh we may pass more extensions in the future?
There was a problem hiding this comment.
This is the streaming client. The requests go over web sockets so the trace info is on the extensions since there are no headers.
There was a problem hiding this comment.
Added the suggested fix
| document: GraphQLQuery, | ||
| operationName: Option[String] = none, | ||
| variables: Option[JsonObject] = none, | ||
| extensions: Option[JsonObject] = none, |
There was a problem hiding this comment.
It would be nice to support extensions here, but this is another concern for another PR.
A more or less one-to-one conversion of the NatchezMiddleware to Otel4sMiddleware. Also includes propagating W3C trace context to connect client spans to server spans as suggested at https://ochenashko.com/practical-observability-distributed-tracing/#6-cross-service-propagation