feat: create client span on request send#253
Conversation
emiliano-at-prima
force-pushed
the
DEVEX-2966/task/no-client-span-is-created-before-making-a-request
branch
from
dab02b0 to
4333d7e
Compare
emiliano-at-prima
force-pushed
the
DEVEX-2966/task/no-client-span-is-created-before-making-a-request
branch
from
4333d7e to
57a9fa5
Compare
cpiemontese
left a comment
cpiemontese
left a comment
There was a problem hiding this comment.
TODOs aside +1 from me, but with otel in Rust who knows
MaeIsBad
left a comment
MaeIsBad
left a comment
There was a problem hiding this comment.
We could add a whole bunch of other attributes, but here are the things I'd like addressed before merging
| // is configurable and defaults to the request method | ||
| // TODO: server.address should be probably set after the host is resolved | ||
| // and we actually have the server address, we are using the hostname here | ||
| #[cfg(feature = "_any_otel_version")] |
There was a problem hiding this comment.
Sidenote: we should unconditionally support tracing by itself without otel
| ); | ||
|
|
||
| #[cfg(feature = "_any_otel_version")] | ||
| let headers = client_span.in_scope(|| self.get_all_headers()); |
There was a problem hiding this comment.
Instead of instrumenting everything manually like this can't we extract the rest of the function into a separate function(or an async {} block) and .instrument that?
There was a problem hiding this comment.
moved that block to a send_request function in 41b083f types look a bit strange but it compiles
| "http.request.method" = %method.as_str(), | ||
| "server.address" = %url.host().map(|h| h.to_string()).unwrap_or_default(), | ||
| "server.port" = %url.port_or_known_default().map(|p| p.to_string()).unwrap_or_default(), | ||
| "url.full" = %url.as_str(), |
There was a problem hiding this comment.
This could include credentials(url.username and url.password), we need to redact those
https://opentelemetry.io/docs/specs/semconv/registry/attributes/url/
| .send() | ||
| .await?; | ||
|
|
||
| let status_code = response.status(); |
| // TODO: the span name on [telepoison](https://github.com/primait/telepoison/blob/b65bcc3bf4ee7744a49ae7ffa040302ab5fe3ce4/lib/telepoison.ex#L160) | ||
| // is configurable and defaults to the request method |
There was a problem hiding this comment.
For reference here's what the spec has to say(it agrees with the todo comment, I just think it's useful to have)
https://opentelemetry.io/docs/specs/semconv/http/http-spans/#name
There was a problem hiding this comment.
I'm still looking for a way to set the span name dynamically with tracing, not sure how to do that
There was a problem hiding this comment.
apparently you cannot, but the opentelemetry exporter should receive otel.name so we are good
| let resp_headers = response.headers().clone(); | ||
| let resp_vec = response.bytes().await.map(|b| b.to_vec()); | ||
|
|
||
| Ok::<_, PrimaBridgeError>((status_code, resp_headers, resp_vec)) |
There was a problem hiding this comment.
We should set the span status to error on error(again per the spec)
https://opentelemetry.io/docs/specs/semconv/http/http-spans/#status
This is not required of us though
otel.name will override the span name sent to OpenTelemetry exporters ref: https://docs.rs/tracing-opentelemetry/latest/tracing_opentelemetry/#special-fields
emiliano-at-prima
force-pushed
the
DEVEX-2966/task/no-client-span-is-created-before-making-a-request
branch
from
d33b4ea to
99191c6
Compare
MaeIsBad
left a comment
MaeIsBad
left a comment
There was a problem hiding this comment.
Would be nice to have test infra for tracing in this repo, but that's out of scope here
3 participants
Still a work in progress, but I need an early feedback because the tracing rust ecosystem is still a bit obscure to me