Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adopt latest semantic conventions #174

Merged
merged 2 commits into from
Jun 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 22 additions & 6 deletions lib/telepoison.ex
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,18 @@ defmodule Telepoison do
alias OpenTelemetry.Tracer
alias Telepoison.Configuration

@http_url Atom.to_string(Conventions.http_url())
@http_method Atom.to_string(Conventions.http_method())
@http_request_method "http.request.method"
@http_response_status_code "http.response.status_code"
@http_route Atom.to_string(Conventions.http_route())
@http_status_code Atom.to_string(Conventions.http_status_code())
@http_url Atom.to_string(Conventions.http_url())
@net_peer_name Atom.to_string(Conventions.net_peer_name())
@server_address "server.address"
@server_port "server.port"
@url_full "url.full"
@url_scheme "url.scheme"
@url_template "url.template"

@doc ~S"""
Configures Telepoison using the provided `opts` `Keyword list`.
Expand Down Expand Up @@ -151,22 +158,22 @@ defmodule Telepoison do

span_name = Keyword.get_lazy(opts, :ot_span_name, fn -> default_span_name(request) end)

%URI{host: host} = request.url |> process_request_url() |> URI.parse()
%URI{scheme: scheme, host: host, port: port} = request.url |> process_request_url() |> URI.parse()

resource_route_attribute =
opts
|> Keyword.get(:ot_resource_route, :unset)
|> get_resource_route(request)
|> case do
resource_route when is_binary(resource_route) ->
[{@http_route, resource_route}]
[{@http_route, resource_route}, {@url_template, resource_route}]

nil ->
[]
end

ot_attributes =
get_standard_ot_attributes(request, host) ++
get_standard_ot_attributes(request, scheme, host, port) ++
get_ot_attributes(opts) ++
resource_route_attribute

Expand Down Expand Up @@ -195,6 +202,7 @@ defmodule Telepoison do
Tracer.set_status(:error, "")
end

Tracer.set_attribute(@http_response_status_code, status_code)
Tracer.set_attribute(@http_status_code, status_code)
end_span()
status_code
Expand All @@ -220,14 +228,22 @@ defmodule Telepoison do
Tracer.set_current_span(ctx)
end

defp get_standard_ot_attributes(request, host) do
defp get_standard_ot_attributes(request, scheme, host, port) do
[
{@http_method,
request.method
|> Atom.to_string()
|> String.upcase()},
{@http_request_method,
request.method
|> Atom.to_string()
|> String.upcase()},
{@http_url, strip_uri_credentials(request.url)},
{@net_peer_name, host}
{@net_peer_name, host},
{@server_address, host},
{@server_port, port},
{@url_full, strip_uri_credentials(request.url)},
{@url_scheme, scheme}
]
end

Expand Down
26 changes: 24 additions & 2 deletions test/telepoison_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,28 @@ defmodule TelepoisonTest do
assert_receive {:span, span(attributes: attributes_record, name: "GET")}
attributes = elem(attributes_record, 4)

assert ["http.method", "http.status_code", "http.url", "net.peer.name"] ==
assert [
"http.method",
"http.request.method",
"http.response.status_code",
"http.status_code",
"http.url",
"net.peer.name",
"server.address",
"server.port",
"url.full",
"url.scheme"
] ==
attributes |> Map.keys() |> Enum.sort()

assert {"http.method", "GET"} in attributes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could maybe start adding comments to attributes that should be deprecated, WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYM? How?

Copy link
Contributor

@mbusi mbusi Jun 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about simple plain comments like

# deprecated

😆

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So # deprecated after each old convention in this list?

@http_method Atom.to_string(Conventions.http_method())
@http_request_method "http.request.method"
@http_response_status_code "http.response.status_code"
@http_route Atom.to_string(Conventions.http_route())
@http_status_code Atom.to_string(Conventions.http_status_code())
@http_url Atom.to_string(Conventions.http_url())
@net_peer_name Atom.to_string(Conventions.net_peer_name())
@server_address "server.address"
@server_port "server.port"
@url_full "url.full"
@url_scheme "url.scheme"
@url_template "url.template"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, something like that 🤷

assert {"http.request.method", "GET"} in attributes
assert {"http.response.status_code", 200} in attributes
assert {"net.peer.name", "localhost"} in attributes
assert {"server.address", "localhost"} in attributes
assert {"server.port", 8000} in attributes
assert {"url.full", "http://localhost:8000"} in attributes
assert {"url.scheme", "http"} in attributes
end

test "traceparent header is injected when no headers" do
Expand Down Expand Up @@ -57,11 +74,12 @@ defmodule TelepoisonTest do
assert "atom" in Enum.map(headers, &elem(&1, 0))
end

test "http.url doesn't contain credentials" do
test "http.url and url.full don't contain credentials" do
Telepoison.get!("http://user:pass@localhost:8000/user/edit/24")

assert_receive {:span, span(attributes: attributes)}, 1000
assert confirm_attributes(attributes, {"http.url", "http://localhost:8000/user/edit/24"})
assert confirm_attributes(attributes, {"url.full", "http://localhost:8000/user/edit/24"})
end
end

Expand All @@ -78,6 +96,7 @@ defmodule TelepoisonTest do

assert_receive {:span, span(attributes: attributes)}, 1000
assert confirm_attributes(attributes, {"http.route", "/user/edit"})
assert confirm_attributes(attributes, {"url.template", "/user/edit"})
end

test "resource route can be explicitly passed to Telepoison invocation as a function" do
Expand All @@ -87,6 +106,7 @@ defmodule TelepoisonTest do

assert_receive {:span, span(attributes: attributes)}, 1000
assert confirm_attributes(attributes, {"http.route", "/user/edit/24"})
assert confirm_attributes(attributes, {"url.template", "/user/edit/24"})
end

test "resource route inference can be explicitly ignored" do
Expand Down Expand Up @@ -121,6 +141,7 @@ defmodule TelepoisonTest do

assert_receive {:span, span(attributes: attributes)}, 1000
assert confirm_attributes(attributes, {"http.route", "/user/edit"})
assert confirm_attributes(attributes, {"url.template", "/user/edit"})
assert confirm_attributes(attributes, {"app.callname", "mariorossi"})
end
end
Expand Down Expand Up @@ -297,6 +318,7 @@ defmodule TelepoisonTest do

defp confirm_http_route_attribute(attributes, value) do
confirm_attributes(attributes, {"http.route", value})
confirm_attributes(attributes, {"url.template", value})
end

defp confirm_http_route_attribute(attributes) do
Expand Down
Loading