Allow wildcard for view instrument name

open-telemetry · Sep 5, 2023 · 8ce874b · 8ce874b
1 parent a673c0f
commit 8ce874b
Show file tree

Hide file tree

Showing 3 changed files with 84 additions and 32 deletions.
diff --git a/apps/opentelemetry_experimental/src/otel_meter_server.erl b/apps/opentelemetry_experimental/src/otel_meter_server.erl
@@ -177,9 +177,7 @@ init([Name, RegName, Resource, Config]) ->
     %% TODO: don't do this if its already set?
     opentelemetry_experimental:set_default_meter(Name, {otel_meter_default, Meter}),
 
-    %% TODO: drop View if Criteria is a wildcard instrument name and View
-    %% name is not undefined
-    Views = [new_view(V) || V <- maps:get(views, Config, [])],
+    Views = lists:filtermap(fun new_view/1, maps:get(views, Config, [])),
 
     {ok, #state{shared_meter=Meter,
                 instruments_tab=InstrumentsTab,
@@ -235,10 +233,7 @@ handle_call({add_view, Name, Criteria, Config}, _From, State=#state{views=Views,
                                                                     callbacks_tab=CallbacksTab,
                                                                     view_aggregations_tab=ViewAggregationsTab,
                                                                     readers=Readers}) ->
-    %% TODO: drop View if Criteria is a wildcard instrument name and View name is not undefined
-    NewView = otel_view:new(Name, Criteria, Config),
-    _ = update_view_aggregations(InstrumentsTab, CallbacksTab, ViewAggregationsTab, [NewView], Readers),
-    {reply, true, State#state{views=[NewView | Views]}};
+    add_view_(Name, Criteria, Config, InstrumentsTab, CallbacksTab, ViewAggregationsTab, Readers, Views, State);
 handle_call(force_flush, _From, State=#state{readers=Readers}) ->
     [otel_metric_reader:collect(Pid) || #reader{pid=Pid} <- Readers],
     {reply, ok, State}.
@@ -258,6 +253,16 @@ code_change(State) ->
 
 %%
 
+-dialyzer({nowarn_function,add_view_/9}).
+add_view_(Name, Criteria, Config, InstrumentsTab, CallbacksTab, ViewAggregationsTab, Readers, Views, State) ->
+    case otel_view:new(Name, Criteria, Config) of
+        {ok, NewView} -> 
+            _ = update_view_aggregations(InstrumentsTab, CallbacksTab, ViewAggregationsTab, [NewView], Readers),
+            {reply, true, State#state{views=[NewView | Views]}};
+        error ->
+            {reply, false, State}
+    end.
+
 instruments_tab(Name) ->
     ets:new(list_to_atom(lists:concat([instruments, "_", Name])), [set,
                                                                    named_table,
@@ -290,11 +295,14 @@ new_view(ViewConfig) ->
     AttributeKeys = maps:get(attribute_keys, ViewConfig, undefined),
     AggregationModule = maps:get(aggregation_module, ViewConfig, undefined),
     AggregationOptions = maps:get(aggregation_options, ViewConfig, #{}),
-    otel_view:new(Name, Selector, #{description => Description,
+    case otel_view:new(Name, Selector, #{description => Description,
                                     attribute_keys => AttributeKeys,
                                     aggregation_module => AggregationModule,
                                     aggregation_options => AggregationOptions
-                                   }).
+                                   }) of
+        {ok, View} -> {true, View};
+        error -> false
+    end.
 
 %% Match the Instrument to views and then store a per-Reader aggregation for the View
 add_instrument_(InstrumentsTab, CallbacksTab, ViewAggregationsTab, Instrument=#instrument{meter=Meter,

diff --git a/apps/opentelemetry_experimental/src/otel_view.erl b/apps/opentelemetry_experimental/src/otel_view.erl
@@ -21,6 +21,7 @@
          new/3,
          match_instrument_to_views/2]).
 
+-include_lib("kernel/include/logger.hrl").
 -include_lib("opentelemetry_api_experimental/include/otel_metrics.hrl").
 -include_lib("opentelemetry_api/include/opentelemetry.hrl").
 -include("otel_metrics.hrl").
@@ -50,11 +51,27 @@
 
 -include_lib("opentelemetry_api/include/gradualizer.hrl").
 
-%% no name means Instrument name is used
-%% must reject wildcard Criteria  in this case
+
 -dialyzer({nowarn_function,new/2}).
--spec new(criteria() | undefined, config()) -> t().
+-spec new(criteria() | undefined, config()) -> {ok, t()} | error.
 new(Criteria, Config) ->
+    new(undefined, Criteria, Config).
+
+-dialyzer({nowarn_function,new/3}).
+-spec new(name(), criteria() | undefined, config()) -> {ok, t()} | error.
+new(undefined, Criteria, Config) ->
+    {ok, do_new(Criteria, Config)};
+new(Name, #{instrument_name := '*'}, _Config) ->
+    ?LOG_INFO("Wildacrd Views can not have a name, discarding view ~s", [Name]),
+    error;
+new(Name, Criteria, Config) ->
+    View = do_new(Criteria, Config),
+    {ok, View#view{name=Name}}.
+
+%% no name means Instrument name is used
+%% must reject wildcard Criteria  in this case
+-dialyzer({nowarn_function,do_new/2}).
+do_new(Criteria, Config) ->
     CriteriaInstrumentName = view_name_from_criteria(Criteria),
     Matchspec = criteria_to_instrument_matchspec(Criteria),
     %% no name given so use the name of the instrument in the selection
@@ -66,14 +83,6 @@ new(Criteria, Config) ->
           aggregation_module=maps:get(aggregation_module, Config, undefined),
           aggregation_options=maps:get(aggregation_options, Config, #{})}.
 
--dialyzer({nowarn_function,new/3}).
--spec new(name(), criteria() | undefined, config()) -> t().
-new(undefined, Criteria, Config) ->
-    new(Criteria, Config);
-new(Name, Criteria, Config) ->
-    View = new(Criteria, Config),
-    View#view{name=Name}.
-
 -dialyzer({nowarn_function,match_instrument_to_views/2}).
 -spec match_instrument_to_views(otel_instrument:t(), [t()]) -> [{t() | undefined, #view_aggregation{}}].
 match_instrument_to_views(Instrument=#instrument{name=InstrumentName,
@@ -128,10 +137,12 @@ value_or(Value, _Other) ->
     Value.
 
 -dialyzer({nowarn_function,criteria_to_instrument_matchspec/1}).
--spec criteria_to_instrument_matchspec(map() | undefined) -> ets:compiled_match_spec().
+-spec criteria_to_instrument_matchspec(map() | undefined) -> ets:comp_match_spec().
 criteria_to_instrument_matchspec(Criteria) when is_map(Criteria) ->
     Instrument =
-      maps:fold(fun(instrument_name, InstrumentName, InstrumentAcc) ->
+      maps:fold(fun(instrument_name, '*', InstrumentAcc) ->
+                        InstrumentAcc;
+                   (instrument_name, InstrumentName, InstrumentAcc) ->
                         InstrumentAcc#instrument{name=InstrumentName};
                    (instrument_kind, Kind, InstrumentAcc) ->
                         InstrumentAcc#instrument{kind=Kind};
@@ -174,6 +185,9 @@ update_meter_schema_url(SchemaUrl, {_, Meter=#meter{instrumentation_scope=Scope}
     {'_', Meter#meter{instrumentation_scope=Scope#instrumentation_scope{schema_url=SchemaUrl}}}.
 
 view_name_from_criteria(Criteria) when is_map(Criteria) ->
-    maps:get(instrument_name, Criteria, undefined);
+    case maps:get(instrument_name, Criteria, undefined) of
+        '*' -> undefined;
+        Name -> Name
+    end;
 view_name_from_criteria(_) ->
     undefined.
diff --git a/apps/opentelemetry_experimental/test/otel_metrics_SUITE.erl b/apps/opentelemetry_experimental/test/otel_metrics_SUITE.erl
@@ -64,21 +64,21 @@
 -define(assertNotReceive(Name, Description, Unit),
         (fun() ->
                  receive
-                     M=#metric{name=MetricName,
-                               description=MetricDescription,
-                               unit=MetricUnit}
+                     {otel_metric, M=#metric{name=MetricName,
+                                             description=MetricDescription,
+                                             unit=MetricUnit}}
                        when MetricName =:= Name,
                             MetricDescription =:= Description,
                             MetricUnit =:= Unit ->
                          ct:fail({metric_received, M})
                  after
-                     0 ->
+                     50 ->
                          ok
                  end
          end)()).
 
 all() ->
-    [default_view, provider_test, view_creation_test, counter_add, multiple_readers,
+    [default_view, provider_test, view_creation_test, wildcard_view, counter_add, multiple_readers,
      explicit_histograms, delta_explicit_histograms, delta_counter, cumulative_counter,
      kill_reader, kill_server, observable_counter, observable_updown_counter, observable_gauge,
      multi_instrument_callback, using_macros, float_counter, float_updown_counter, float_histogram,
@@ -433,17 +433,17 @@ view_creation_test(_Config) ->
 
     ?assert(otel_meter_server:add_view(view_a, #{instrument_name => a_counter}, #{aggregation_module => otel_aggregation_sum})),
 
-    View = otel_view:new(#{instrument_name => a_counter}, #{aggregation_module => otel_aggregation_sum}),
+    {ok, View} = otel_view:new(#{instrument_name => a_counter}, #{aggregation_module => otel_aggregation_sum}),
     %% view name becomes the instrument name
     ?assertEqual(a_counter, View#view.name),
 
     Matches = otel_view:match_instrument_to_views(Counter, [View]),
     ?assertMatch([_], Matches),
 
-    ViewUnitMatch = otel_view:new(#{instrument_name => CounterName, instrument_unit => CounterUnit}, #{aggregation_module => otel_aggregation_sum}),
+    {ok, ViewUnitMatch} = otel_view:new(#{instrument_name => CounterName, instrument_unit => CounterUnit}, #{aggregation_module => otel_aggregation_sum}),
     ?assertMatch([{#view{}, _}], otel_view:match_instrument_to_views(Counter, [ViewUnitMatch])),
 
-    ViewUnitNotMatch = otel_view:new(#{instrument_name => CounterName, instrument_unit => not_matching}, #{aggregation_module => otel_aggregation_sum}),
+    {ok, ViewUnitNotMatch} = otel_view:new(#{instrument_name => CounterName, instrument_unit => not_matching}, #{aggregation_module => otel_aggregation_sum}),
     ?assertMatch([{undefined, _}], otel_view:match_instrument_to_views(Counter, [ViewUnitNotMatch])),
 
     %% views require a unique name
@@ -459,6 +459,36 @@ view_creation_test(_Config) ->
 
     ok.
 
+wildcard_view(_Config) ->
+    Meter = opentelemetry_experimental:get_meter(),
+
+    ViewCriteria = #{instrument_name => '*'},
+    ViewConfig = #{aggregation_module => otel_aggregation_drop},
+
+    ?assert(otel_meter_server:add_view(ViewCriteria, ViewConfig)),
+
+    CounterName = a_counter,
+    CounterDesc = <<"counter description">>,
+    CounterUnit = kb,
+
+    Counter = otel_counter:create(Meter, CounterName,
+                                  #{description => CounterDesc,
+                                    unit => CounterUnit}),
+
+    ?assertEqual(ok, otel_counter:add(Counter, 1, #{})),
+
+    otel_meter_server:force_flush(),
+
+    ?assertNotReceive(CounterName, CounterDesc, CounterUnit),
+
+    {ok, View} = otel_view:new(ViewCriteria, ViewConfig),
+    ?assertMatch([{#view{}, _}], otel_view:match_instrument_to_views(Counter, [View])),
+
+    %% not possible to create wildcard views with a name
+    error = otel_view:new(view_name, ViewCriteria, ViewConfig),
+
+    ok.
+
 counter_add(_Config) ->
     Meter = opentelemetry_experimental:get_meter(),
 
@@ -825,7 +855,7 @@ kill_server(_Config) ->
     otel_meter_server:force_flush(),
 
     %% at this time a crashed meter server will mean losing the recorded metrics up to that point
-    ?assertNotReceive(a_counter, <<"counter description">>, kb),
+    ?assertSumReceive(a_counter, <<"counter description">>, kb, []),
     ?assertSumReceive(z_counter, <<"counter description">>, kb, [{9, #{<<"c">> => <<"b">>}}]),
 
     ok.