Req 1.27 SemConv #375
Req 1.27 SemConv #375
Conversation
|
If it wasn't 1.0 then it wasn't stable. No need to do a major version bump if it was 0.x. |
| # propagate_trace_headers_fn: [ | ||
| # type: :mfa, | ||
| # default: {__MODULE__, :default_propagation_fn, []}, | ||
| # doc: "Default function returns `false`. See docs for more info" | ||
| # ], |
There was a problem hiding this comment.
Is this commented-out code just here for our reference as developers of this library, or was it accidentally not removed? It feels weird to me that it's there but commented-out if it's just there as private documentation.
|
|
||
| if response.status >= 400 do | ||
| OpenTelemetry.Tracer.set_status(OpenTelemetry.status(:error, "")) | ||
| Tracer.set_status(OpenTelemetry.status(:error, "")) |
There was a problem hiding this comment.
Looks like it was already like this, but IMO it shouldn't be an error span necessarily just because the server successfully returned a 404 for example, if that's what the caller expected. In that case, the client did its job correctly, so there was no error. In fact, the server that the client called probably did as well unless it was a 5xx error, so it shouldn't be an error on its span either. Does OTel say we should do it this way for HTTP clients, or is this just something we've chosen to do here?
There was a problem hiding this comment.
It's an error for HTTP clients.
For HTTP status codes in the 4xx range span status MUST be left unset in case of SpanKind.SERVER
and SHOULD be set to Error in case of SpanKind.CLIENT.
https://hexdocs.pm/opentelemetry_semantic_conventions/http-spans.html#status
There was a problem hiding this comment.
IMO that's a bummer and not something I would want to have in my traces personally, so I guess that's why it's a SHOULD instead of a MUST. But as a library, I guess we have to decide whether we think it's right to follow that convention, since it's not required. My vote would be to not set it as an error unless it was like a timeout or connection error or something where there's no response at all. If it's a 5xx, I would be fine with it being marked as an error in the client, just to call attention to it even though it's not Req's fault if the server returns a 504 or something, but I think it's totally normal for 404s to happen all the time when you're just querying an API for "does this thing exist?" and it just says "no" as a 404 - you don't need to call special attention to that.
There was a problem hiding this comment.
Users are allowed to mark a span ok so they can hook into the request and mark it should they choose to. We could look at adding a response step to let folks supply a function but this PR isn't seeking to change any existing functionality. I think that's a fine feature request.
There was a problem hiding this comment.
Yep, that makes sense! ✅
| url: "/" | ||
| ) | ||
|
|
||
| assert_receive {:span, span(name: :GET)} |
There was a problem hiding this comment.
Is it weird that the name is an atom here and a binary when the path params are parsed? Feels surprising to me.
There was a problem hiding this comment.
We don't to_atom anything for users as a general rule. I'm not sure I'd want to deviate from it since sobelow would probably gripe but I don't know that for a fact. What do you think?
There was a problem hiding this comment.
Right, I don't think we'd convert to an atom, but I was kind of expecting that the span name would always be a string.
There was a problem hiding this comment.
Atoms are dirt cheap. A string is always heavier and requires memory allocation. It's also an implementation detail that shouldn't concern the user. This only arises if they don't set the path params.
I wouldn't mind adding an efficiency option to allow creating atoms but I don't know how sobelow would treat that. Suppose we could always just put the ignore statement in.
There was a problem hiding this comment.
Similar to the other comment, I don't think this is a blocker if that's already how it was working before, but I agree that it would be something worth considering as an option in a future PR where we can focus on just that aspect.
bryannaegele
force-pushed
the
req-1.26-semconv
branch
from
b8afd41
to
9ccf176
Compare
Co-authored-by: Greg Mefford <[email protected]>
Co-authored-by: Greg Mefford <[email protected]>
bryannaegele
force-pushed
the
req-1.26-semconv
branch
from
0a327da
to
4337c0d
Compare
Updates Req to SemConv 1.26. This creates a number of breaking changes in order to comply with the new SemConv.
Given previous stability, breaking changes, and otel http semconv stability, this will mark a 1.0 release, as well.
Addresses #183 for Req
Resolves #351
Resolves #363