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.

Sign up for GitHub

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

🐛 Fix OpentelemetryFinch handler on Finch.stream_while #398

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

🐛 Fix OpentelemetryFinch handler on Finch.stream_while #398

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7e0208c
🐛 Fix handler on streams
rparcus Oct 18, 2024
91808de
🎨 cleanup
rparcus Oct 18, 2024
2fac77e
Merge branch 'main' into fix_for_finch_stream_while
rparcus Nov 20, 2024
9ac5b88
♻️ refactor meta handler to match on status presence
rparcus Dec 3, 2024
fb5d2d1
Merge branch 'main' into fix_for_finch_stream_while
rparcus Dec 3, 2024
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
13 changes: 11 additions & 2 deletions instrumentation/opentelemetry_finch/lib/opentelemetry_finch.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,17 @@ defmodule OpentelemetryFinch do

status =
case meta.result do
{:ok, response} -> response.status
_ -> 0
{:ok, response} when is_map(response) ->
Copy link
Author

Choose a reason for hiding this comment

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

The main issue was here. Not every response contains a status key as it depends on the implementation of the streaming callback functions.

A fallback case is also added. In this fix, I'm not trying to come up with a general solution but just one that works for the patterns I saw. The tests use callbacks that return values in the patterns I was already working with.

Copy link

@lud-wj lud-wj Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I guess you could just match on the presence of the status:

case meta.result do
  {:ok, %{status: status}} when is_integer(status) -> status
  {:ok, {_request, %{status: status}}} when is_integer(status) -> status
  {:ok, {status, _headers, _body}} when is_integer(status) -> status
  _ -> 0
end

Because there is a risk that even when response is a map, response.status may not be set. As far as I can tell that part of the event is the stream accumulator and it could be anything, any term.

Copy link

@lud-wj lud-wj Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

(edit fixed my clauses 😅 )

response.status

{:ok, {_request, response}} when is_map(response) ->
response.status

{:ok, {status, _headers, _body}} when is_integer(status) ->
status

_ ->
0
end

url = build_url(meta.request.scheme, meta.request.host, meta.request.port, meta.request.path)
Expand Down
110 changes: 110 additions & 0 deletions instrumentation/opentelemetry_finch/test/opentelemetry_finch_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,110 @@ defmodule OpentelemetryFinchTest do
} = :otel_attributes.map(attributes)
end

test "records span on streamed requests where the acc is a 3 elements tuple", %{
bypass: bypass,
test: finch_name
} do
Bypass.expect_once(bypass, "GET", "/", fn conn ->
Plug.Conn.send_resp(conn, 200, "OK")
end)

OpentelemetryFinch.setup()

_conn = start_supervised!({Finch, name: finch_name})

acc = {nil, [], ""}

fun = fn
{:status, value}, {_, headers, body} -> {:cont, {value, headers, body}}
{:headers, value}, {status, headers, body} -> {:cont, {status, headers ++ value, body}}
{:data, value}, {status, headers, body} -> {:cont, {status, headers, body <> value}}
end

assert {:ok, {200, [_ | _], "OK"}} =
Finch.build(:get, endpoint_url(bypass.port))
|> Finch.stream_while(finch_name, acc, fun)

assert_receive {:span,
span(
name: "HTTP GET",
kind: :client,
attributes: attributes
)}

assert %{
"net.peer.name": "localhost",
"http.method": "GET",
"http.target": "/",
"http.scheme": :http,
"http.status_code": 200
} = :otel_attributes.map(attributes)
end

test "records span on streamed requests where the acc is a 2 elements tuple", %{
bypass: bypass,
test: finch_name
} do
Bypass.expect_once(bypass, "GET", "/", fn conn ->
Plug.Conn.send_resp(conn, 200, "OK")
end)

OpentelemetryFinch.setup()

_conn = start_supervised!({Finch, name: finch_name})

acc = {nil, %{body: "", headers: %{}, trailers: [], status: 200}}

fun = fn
{:status, status}, {req, resp} ->
{:cont, {req, %{resp | status: status}}}

{:headers, fields}, {req, resp} ->
fields = finch_fields_to_map(fields)
resp = update_in(resp.headers, &Map.merge(&1, fields))
{:cont, {req, resp}}

{:data, data}, acc ->
{:cont, acc}

{:trailers, fields}, {req, resp} ->
fields = finch_fields_to_map(fields)
resp = update_in(resp.trailers, &Map.merge(&1, fields))
{:cont, {req, resp}}
end

assert {:ok,
{_request,
%{
status: 200,
body: "",
headers: %{
"cache-control" => _,
"content-length" => ["2"],
"date" => [_],
"server" => _
},
trailers: []
}}} =
Finch.build(:get, endpoint_url(bypass.port))
|> Finch.stream_while(finch_name, acc, fun)

assert_receive {:span,
span(
name: "HTTP GET",
kind: :client,
attributes: attributes
)}

assert %{
"net.peer.name": "localhost",
"http.method": "GET",
"http.target": "/",
"http.scheme": :http,
"http.status_code": 200
} = :otel_attributes.map(attributes)
end

test "records span on requests failed", %{bypass: _} do
OpentelemetryFinch.setup()

Expand All @@ -76,4 +180,10 @@ defmodule OpentelemetryFinchTest do
end

defp endpoint_url(port), do: "http://localhost:#{port}/"

defp finch_fields_to_map(fields) do
Enum.reduce(fields, %{}, fn {name, value}, acc ->
Map.update(acc, name, [value], &(&1 ++ [value]))
end)
end
end
Loading