create unique processor name in otel_tracer_server

Each processor needs a unique name to reference but always creating it in start_link means everytime the process restarts it creates a new atom and new persistent term key. This is a leak. This patch moves the atom creation to otel_tracer_server so it is only done once.
open-telemetry · Nov 6, 2023 · d4d5c21 · d4d5c21
1 parent 0ec0e86
commit d4d5c21
Show file tree

Hide file tree

Showing 4 changed files with 24 additions and 24 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -25,11 +25,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - [Attributes module `otel_attributes` moved to
   API](https://github.com/open-telemetry/opentelemetry-erlang/pull/618)
 
+### Fixes
+
+- [Fix leak of atoms/persistent terms by creating unique processor name in `otel_tracer_server`](https://github.com/open-telemetry/opentelemetry-erlang/pull/646)
+
 ## Experimental API
 
 ### Changes
 
-- [Allow to create observable instruments without passing callback arguments](https://github.com/open-telemetry/opentelemetry-erlang/pull/604)
+- [Allow to create observable instruments without passing callback
+  arguments](https://github.com/open-telemetry/opentelemetry-erlang/pull/604)
 - [Allow to give `advisory_params` to instrument creation functions](https://github.com/open-telemetry/opentelemetry-erlang/pull/628)
 
 ## Experimental SDK

diff --git a/apps/opentelemetry/src/otel_batch_processor.erl b/apps/opentelemetry/src/otel_batch_processor.erl
@@ -90,17 +90,11 @@ current_tab_to_list(RegName) ->
     ets:tab2list(?CURRENT_TABLE(RegName)).
 -endif.
 
-start_link(Config) ->
-    Name = case maps:find(name, Config) of
-               {ok, N} ->
-                   N;
-               error ->
-                   %% use a unique reference to distiguish multiple batch processors while
-                   %% still having a single name, instead of a possibly changing pid, to
-                   %% communicate with the processor
-                   erlang:ref_to_list(erlang:make_ref())
-           end,
-
+%% require a unique name to distiguish multiple batch processors while
+%% still having a single name, instead of a possibly changing pid, to
+%% communicate with the processor
+-spec start_link(#{name := atom() | list()}) -> {ok, pid(), map()}.
+start_link(Config=#{name := Name}) ->
     RegisterName = ?REG_NAME(Name),
     Config1 = Config#{reg_name => RegisterName},
     {ok, Pid} = gen_statem:start_link({local, RegisterName}, ?MODULE, [Config1], []),

diff --git a/apps/opentelemetry/src/otel_simple_processor.erl b/apps/opentelemetry/src/otel_simple_processor.erl
@@ -64,17 +64,11 @@
 -define(DEFAULT_EXPORTER_TIMEOUT_MS, timer:minutes(5)).
 -define(NAME_TO_ATOM(Name, Unique), list_to_atom(lists:concat([Name, "_", Unique]))).
 
-start_link(Config) ->
-    Name = case maps:find(name, Config) of
-               {ok, N} ->
-                   N;
-               error ->
-                   %% use a unique reference to distiguish multiple batch processors while
-                   %% still having a single name, instead of a possibly changing pid, to
-                   %% communicate with the processor
-                   erlang:ref_to_list(erlang:make_ref())
-           end,
-
+%% require a unique name to distiguish multiple simple processors while
+%% still having a single name, instead of a possibly changing pid, to
+%% communicate with the processor
+-spec start_link(#{name := atom() | list()}) -> {ok, pid(), map()}.
+start_link(Config=#{name := Name}) ->
     RegisterName = ?NAME_TO_ATOM(?MODULE, Name),
     Config1 = Config#{reg_name => RegisterName},
     {ok, Pid} = gen_statem:start_link({local, RegisterName}, ?MODULE, [Config1], []),

diff --git a/apps/opentelemetry/src/otel_tracer_server.erl b/apps/opentelemetry/src/otel_tracer_server.erl
@@ -159,7 +159,14 @@ init_processor(SpanProcessorSup, ProcessorModule, Config) ->
     %% start_link is an optional callback for processors
     case lists:member({start_link, 1}, ProcessorModule:module_info(exports)) of
         true ->
-            try supervisor:start_child(SpanProcessorSup, [ProcessorModule, Config]) of
+            try supervisor:start_child(SpanProcessorSup,
+                                       [ProcessorModule,
+                                        %% use a unique reference to distiguish multiple processors of the same type while
+                                        %% still having a single name, instead of a possibly changing pid, to
+                                        %% communicate with the processor
+                                        maps:merge(#{name => erlang:ref_to_list(erlang:make_ref())},
+                                                   Config)])
+            of
                 {ok, _Pid, Config1} ->
                     {true, {ProcessorModule, Config1}};
                 {error, Reason} ->