Skip to content

Commit

Permalink
Close spans on error (#979)
Browse files Browse the repository at this point in the history
When our `Appsignal.instrument` function encounters an error when the
function argument raises an error, the span it created would not be
closed.

Wrap the function call in a `try` block, and move the close logic to an
`after` block to ensure that the span is always closed.

I don't know if `instrument_root` is actually used anywhere. We don't
call it anywhere and we haven't documented it, but I'm updating it just
in case to match the `instrument` helper function.

Related internal thread:
https://appsignal.slack.com/archives/C7XHXQ7N0/p1739893354569399?thread_ts=1739870246.417719&cid=C7XHXQ7N0
  • Loading branch information
tombruijn authored Feb 19, 2025
1 parent 23046c0 commit d218b40
Show file tree
Hide file tree
Showing 3 changed files with 92 additions and 5 deletions.
14 changes: 14 additions & 0 deletions .changesets/close-spans-on-error.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
---
bump: patch
type: fix
---

Close instrumentation spans when an error occurs inside the `Appsignal.instrument` helper's function argument. This prevents spans and traces from not being closed properly.

This will no longer fail to close spans:

```elixir
Appsignal.instrument("event name", fn -> do
raise "Oh no!"
end)
```
17 changes: 12 additions & 5 deletions lib/appsignal/instrumentation.ex
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,12 @@ defmodule Appsignal.Instrumentation do
def instrument(fun) do
span = @tracer.create_span("background_job", @tracer.current_span)

result = call_with_optional_argument(fun, span)
@tracer.close_span(span)
result =
try do
call_with_optional_argument(fun, span)
after
@tracer.close_span(span)
end

result
end
Expand Down Expand Up @@ -68,9 +72,12 @@ defmodule Appsignal.Instrumentation do
|> @span.set_name(name)
|> @span.set_attribute("appsignal:category", name)

result = fun.()

@tracer.close_span(span)
result =
try do
call_with_optional_argument(fun, span)
after
@tracer.close_span(span)
end

result
end
Expand Down
66 changes: 66 additions & 0 deletions test/appsignal/instrumentation_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,43 @@ defmodule Appsignal.InstrumentationTest do
end
end

describe "instrument/2, when an error is raised from the function" do
setup do
try do
Appsignal.Instrumentation.instrument("test", fn ->
raise "Exception!"
end)
rescue
e in RuntimeError ->
e

Check warning on line 447 in test/appsignal/instrumentation_test.exs

View workflow job for this annotation

GitHub Actions / test (1.13.x, 24.x)

variable e in code block has no effect as it is never returned (remove the variable or assign it to _ to avoid warnings)

Check warning on line 447 in test/appsignal/instrumentation_test.exs

View workflow job for this annotation

GitHub Actions / test (1.12.x, 24.x)

variable e in code block has no effect as it is never returned (remove the variable or assign it to _ to avoid warnings)

%{
error: e
}
end
end

test "creates a root span" do
assert Test.Tracer.get(:create_span) == {:ok, [{"background_job", nil}]}
end

test "sets the span's name" do
assert {:ok, [{%Span{}, "test"}]} = Test.Span.get(:set_name)
end

test "sets the span's category attribute" do
assert {:ok, [{%Span{}, "appsignal:category", "test"}]} = Test.Span.get(:set_attribute)
end

test "raises the error", %{error: error} do
assert %RuntimeError{message: "Exception!"} = error
end

test "closes the span" do
assert {:ok, [{%Span{}}]} = Test.Tracer.get(:close_span)
end
end

describe "instrument/2, when passing a function that takes an argument" do
setup do
%{return: Appsignal.Instrumentation.instrument("test", fn span -> span end)}
Expand Down Expand Up @@ -516,6 +553,35 @@ defmodule Appsignal.InstrumentationTest do
end
end

describe "instrument_root/3, when an error is raised from the function" do
setup do
try do
Appsignal.Instrumentation.instrument_root("background_job", "name", fn ->
raise "Exception!"
end)
rescue
e in RuntimeError ->
e

Check warning on line 564 in test/appsignal/instrumentation_test.exs

View workflow job for this annotation

GitHub Actions / test (1.13.x, 24.x)

variable e in code block has no effect as it is never returned (remove the variable or assign it to _ to avoid warnings)

Check warning on line 564 in test/appsignal/instrumentation_test.exs

View workflow job for this annotation

GitHub Actions / test (1.12.x, 24.x)

variable e in code block has no effect as it is never returned (remove the variable or assign it to _ to avoid warnings)

%{
error: e
}
end
end

test "creates a root span" do
assert Test.Tracer.get(:create_span) == {:ok, [{"background_job", nil}]}
end

test "raises the error", %{error: error} do
assert %RuntimeError{message: "Exception!"} = error
end

test "closes the span" do
assert {:ok, [{%Span{}}]} = Test.Tracer.get(:close_span)
end
end

describe ".set_error/2, with a root span" do
setup do
span = Tracer.create_span("http_request")
Expand Down

0 comments on commit d218b40

Please sign in to comment.