Add tests and tweak behavior of opentelemetry_req #194
Conversation
The main objective of this patch is to add few tests to cover main points of how this library handles OpenTelemetry. As by-product of those tests, few tweaks were done to production code. More specifically, how we handle `:path_params` validation. `:path_params` only has restrictions when used for span names, due constraints of "span names should never be unbounded". As such, when we set `:span_name` with a custom one, the check is skipped. Another change is how `:no_path_params` imply: here, I understand that we as library users are asserting that "I comply and abide this path has no dynamic parts", and as such, it's safe to just use it instead of fallback to only METHOD. Otherwise, we would need some workaround like set empty :path_params just to ensure "safe" paths are reported correctly on span names. I've considered addting Yet Another Option for "I don't want any path inference" that disables check and set span names just as METHOD, but refrain to do so here to not deviate too much from current implementation.
|
cc @wojtekmach, as I just realized you have some patches in the works when I was prep this for publication. Let me know if there are any conflicts and I'm happy to fix them here. |
| setup do | ||
| :otel_simple_processor.set_exporter(:otel_exporter_pid, self()) | ||
|
|
||
| :ok |
There was a problem hiding this comment.
you may also consider returning:
pid = self()
adapter = fn req ->
send(pid, {:req, req})
{req, Req.Response.new(status: 204)}
end
req =
Req.new(adapter: adapter)
|> OpentelemetryReq.attach()
%{req: req}which would remove some duplication from the tests. But then again such duplication doesn't hurt too much either. :)
|
|
||
| defp span_name(request) do | ||
| case request.options[:span_name] do | ||
| nil -> | ||
| method = http_method(request.method) | ||
|
|
||
| case Req.Request.get_private(request, :path_params_template) do | ||
| nil -> method | ||
| nil -> "#{method} #{request.url.path}" |
There was a problem hiding this comment.
We aren't doing 1.20+ spec piecemeal, so please keep this as it was. All the libs will have to be updated at one time.
The template on line 127 was prematurely merged and not reverted yet, so I can't publish until both are reverted to the behavior that was present prior to that PR.
There was a problem hiding this comment.
The line in question is from 19a44fb
There was a problem hiding this comment.
I've forgotten the plan for 1.20, hehe. Did we write it down somewhere. I vaguely remember. Maybe we should do it "now" if there are ppl looking to contribute towards the goal (not saying this should be in this PR but maybe @andrewhr can open a separate one).
There was a problem hiding this comment.
I'd like to do it all at once with 1.20 semconv. What do you think?
For now I'd like to have at least one more "before" release so nobody is forced to retool to get everything I haven't cut releases for yet.
I'll write it up this morning in an issue
There was a problem hiding this comment.
From my understanding of older versions, the "best effort" of span names were always there, so this doesn't not necessary mark a breaking change from the convention definitions. In other words, was not my intend here to piecemeal implement newer specs. Having said that, I totally understand if we prefer to rollback this change - it just a matter of ergonomics so Spans can be consistent in cases we can assert they aren't dynamic.
Btw, thanks for the write up on #197, that's quite helpful! 🙇
There was a problem hiding this comment.
Can we get this addressed? I'd like to get #184 through and this would be a blocker.
| assert %{ | ||
| "http.url": "http://example.com/users", | ||
| "http.method": :GET, | ||
| "http.scheme": "http", |
There was a problem hiding this comment.
This should be an atom.
There was a problem hiding this comment.
That's just what URI.parse/1 returns, and which is the same behavior of Tesla middleware. Should we still change it?
I thought I have is, in case we still prefer atoms (perf reasons, right?), I can map to few known schemes and fallback to return as string. Does that seems a good compromise?
There was a problem hiding this comment.
It can just be to_atom but we can do that in another PR if it's a big lift. The list of registered schemes is massive.
The fallback is just whatever the path is in the request, not method. Maybe that changes with 1.20+ spec but until then the behavior is just to use the path, e.g. |
I dug back through the history and as of 1.17 the format was |
|
#375 should cover everything |
The main objective of this patch is to add few tests to cover main points of how this library handles OpenTelemetry.
As by-product of those tests, few tweaks were done to production code. More specifically, how we handle
:path_paramsvalidation.:path_paramsonly has restrictions when used for span names, due constraints of "span names should never be unbounded". As such, when we set:span_namewith a custom one, the check is skipped.Another change is how
:no_path_paramsimply: here, I understand that we as library users are asserting that "I comply and abide this path has no dynamic parts", and as such, it's safe to just use it instead of fallback to only METHOD. Otherwise, we would need some workaround like set empty :path_params just to ensure "safe" paths are reported correctly on span names.I've considered addting Yet Another Option for "I don't want any path inference" that disables check and set span names just as METHOD, but refrain to do so here to not deviate too much from current implementation.