set OTLP service name and namespace #210
Conversation
reqwest-tracing still requires tracing-opentelemetry 0.15, so tracing headers do not get propagated if the versions are different
the spans for execute were considered root spans and did not appear under the graphql_request span
| ); | ||
| let query_task = self.query_cache.get_query(&request.query); | ||
| ) | ||
| .instrument(tracing::info_span!(parent: &span, "execution")); |
There was a problem hiding this comment.
I wonder if this instrumentation shouldn't stay in model.rs 🤔 validate_request does that already. Does using the decorator fix the issue? I like the decorator ^_^
#[tracing::instrument(name = "validation", level = "debug")]
pub fn validate_request(There was a problem hiding this comment.
The attribute does not allow overriding the parent span: https://docs.rs/tracing/0.1.29/tracing/attr.instrument.html
There was a problem hiding this comment.
Yes this comment was not about overriding parent span and stuff. More a question of:
- where should go the trace: at the source or at the caller
- is the decorator more esthetic, should we use it, can we
There was a problem hiding this comment.
that topic should go in #213
There was a problem hiding this comment.
I'm fine either way, as long as we make sure we don't serialize too much info with the attribute
| @@ -88,6 +89,7 @@ pub struct ApolloPreparedQuery { | |||
| impl PreparedQuery for ApolloPreparedQuery { | |||
| #[tracing::instrument(level = "debug")] | |||
| async fn execute(self, request: Arc<Request>) -> ResponseStream { | |||
| let span = Span::current(); | |||
| stream::once( | |||
| async move { | |||
There was a problem hiding this comment.
What would happen if we attach the current span to this future? Would that solve the problem? There is a dedicated function for that https://docs.rs/tracing/0.1.29/tracing/trait.Instrument.html#method.in_current_span
There was a problem hiding this comment.
Yep, I tried that, but it makes the future non Send. I think I found another way without setting the parent explicitely, I'll try this afternoon
There was a problem hiding this comment.
I should have thought of that earlier 😅 23020ae
Maybe we can simplify ResponseStream now
There was a problem hiding this comment.
Cool! ^_^ some improvements coming then? I'm glad I looked at this PR even if it was a bit late
There was a problem hiding this comment.
There's always room for improvement 😀
fix #180
related to #38
26045c3 is temporary, I'll send a PR to reqwest-tracing to support the tracing-opentelemetry 0.16
we should get #38 implemented, to make sure those kinds of regressions do not happen