Merge pull request #624 from albertored/wildcard-view

Tristan Sloughter · web-flow · commit 2c45b1b8dc66 · 2023-10-05T04:49:14.000-06:00
Allow wildcard for view instrument name
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -40,6 +40,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - [Validate instrument name](https://github.com/open-telemetry/opentelemetry-erlang/pull/604)
 - [Handle `explict_bucket_boundaries` advisory parameter](https://github.com/open-telemetry/opentelemetry-erlang/pull/628)
 - [Rename `boundaries` to `explict_bucket_boundaries` in histogram explicit aggregation options](https://github.com/open-telemetry/opentelemetry-erlang/pull/628)
+- [Allow creating wildcard views](https://github.com/open-telemetry/opentelemetry-erlang/pull/624)
 
 ### Changes
 
diff --git a/apps/opentelemetry_api_experimental/include/match_spec.hrl b/apps/opentelemetry_api_experimental/include/match_spec.hrl
@@ -0,0 +1,8 @@
+-ifndef(MATCH_SPEC_TYPES_DEFINED).
+
+-type match_var() :: '_' | '$1' | '$2' | '$3' | '$4' | '$5' | '$6' | '$7' | '$8' | '$9'.
+-type match_spec(A) :: A | match_var() | {const, A}.
+
+-define(MATCH_SPEC_TYPES_DEFINED, true).
+
+-endif.
diff --git a/apps/opentelemetry_api_experimental/include/otel_metrics.hrl b/apps/opentelemetry_api_experimental/include/otel_metrics.hrl
@@ -1,13 +1,15 @@
--record(instrument, {module          :: module(),
-                     meter           :: otel_meter:t(),
-                     name            :: otel_instrument:name(),
-                     description     :: otel_instrument:description() | undefined,
-                     kind            :: otel_instrument:kind(),
-                     unit            :: otel_instrument:unit() | undefined,
-                     temporality     :: otel_instrument:temporality(),
-                     callback        :: otel_instrument:callback() | undefined,
-                     callback_args   :: otel_instrument:callback_args() | undefined,
-                     advisory_params :: otel_instrument:advisory_params() | undefined}).
+-include_lib("match_spec.hrl").
+
+-record(instrument, {module          :: match_spec(module()),
+                     meter           :: match_spec(otel_meter:t()),
+                     name            :: match_spec(otel_instrument:name()),
+                     description     :: match_spec(otel_instrument:description()) | undefined,
+                     kind            :: match_spec(otel_instrument:kind()),
+                     unit            :: match_spec(otel_instrument:unit()) | undefined,
+                     temporality     :: match_spec(otel_instrument:temporality()),
+                     callback        :: match_spec(otel_instrument:callback()) | undefined,
+                     callback_args   :: match_spec(otel_instrument:callback_args()) | undefined,
+                     advisory_params :: match_spec(otel_instrument:advisory_params() | undefined)}).
 
 -define(TEMPORALITY_DELTA, temporality_delta).
 -define(TEMPORALITY_CUMULATIVE, temporality_cumulative).
diff --git a/apps/opentelemetry_experimental/include/otel_metrics.hrl b/apps/opentelemetry_experimental/include/otel_metrics.hrl
@@ -1,8 +1,9 @@
+-include_lib("opentelemetry_api_experimental/include/match_spec.hrl").
+
 -define(DEFAULT_METER_PROVIDER, otel_meter_provider_default).
 
--type match_var() :: '_' | '$1' | '$2' | '$3' | '$4' | '$5' | '$6' | '$7' | '$8' | '$9'.
--type match_expr(A) :: undefined | match_var() | {const, A}.
--type match_spec(A) :: match_expr(A).
+-type key_inner_match_spec() :: {match_spec(atom()), match_spec(opentelemetry:attributes_map()), match_spec(reference())}.
+-type key_match_spec() :: match_spec(otel_aggregation:key()) | key_inner_match_spec()  | {key_inner_match_spec()}.
 
 -record(meter,
         {
@@ -26,49 +27,49 @@
 -record(sum_aggregation,
         {
          %% TODO: attributes should be a tuple of just the values, sorted by attribute name
-         key :: otel_aggregation:key() | otel_aggregation:key_match_spec() | {element, 2, '$_'},
-         start_time_unix_nano :: integer() | match_spec(integer()),
-         last_start_time_unix_nano :: integer() | match_spec(integer()),
-         checkpoint :: number() | match_spec(number()) | {'+', '$2', '$3'} | {'+', '$3', '$4'},
-         previous_checkpoint :: number() | match_spec(number()) | {'+', '$5', '$6'},
-         int_value :: number() | match_spec(number()) | {'+', '$3', {const, number()}},
-         float_value :: number() | match_spec(number()) | {'+', '$4', {const, number()}}
+         key :: key_match_spec() | undefined | {element, 2, '$_'},
+         start_time_unix_nano :: match_spec(integer()) | undefined,
+         last_start_time_unix_nano :: match_spec(integer()) | undefined,
+         checkpoint :: match_spec(number()) | undefined | {'+', '$2', '$3'} | {'+', '$3', '$4'},
+         previous_checkpoint :: match_spec(number()) | undefined | {'+', '$5', '$6'},
+         int_value :: match_spec(number()) | undefined | {'+', '$3', {const, number()}},
+         float_value :: match_spec(number()) | undefined | {'+', '$4', {const, number()}}
         }).
 
 -record(last_value_aggregation,
         {
          %% TODO: attributes should be a tuple of just the values, sorted by attribute name
-         key :: otel_aggregation:key() | otel_aggregation:key_match_spec(),
-         checkpoint :: number() | match_spec(number()),
-         value :: number() | match_spec(number()),
-         start_time_unix_nano :: integer() | match_spec(integer()),
-         last_start_time_unix_nano :: integer() | match_spec(integer())
+         key :: key_match_spec() | undefined,
+         checkpoint :: match_spec(number()) | undefined,
+         value :: match_spec(number()) | undefined,
+         start_time_unix_nano :: match_spec(integer()) | undefined,
+         last_start_time_unix_nano :: match_spec(integer()) | undefined
         }).
 
 
 -record(explicit_histogram_checkpoint,
         {
-         bucket_counts :: counters:counters_ref() | match_spec(counters:counters_ref()),
-         min :: number() | match_spec(number()),
-         max :: number() | match_spec(number()),
-         sum :: number() | match_spec(number()),
-         start_time_unix_nano :: integer() | match_spec(number())
+         bucket_counts :: match_spec(counters:counters_ref()) | undefined,
+         min :: match_spec(number()) | undefined,
+         max :: match_spec(number()) | undefined,
+         sum :: match_spec(number()) | undefined,
+         start_time_unix_nano :: match_spec(number()) | undefined
         }).
 
 -record(explicit_histogram_aggregation,
         {
          %% TODO: attributes should be a tuple of just the values, sorted by attribute name
-         key :: otel_aggregation:key() | otel_aggregation:key_match_spec(),
-         start_time_unix_nano :: integer() | {const, eqwalizer:dynamic()} | '$9' | '$2' | undefined,
+         key :: key_match_spec() | undefined,
+         start_time_unix_nano :: match_spec(integer()) | undefined,
          %% instrument_temporality :: otel_aggregation:temporality(),
          %% default: [0.0, 5.0, 10.0, 25.0, 50.0, 75.0, 100.0, 250.0, 500.0, 1000.0]
-         explicit_bucket_boundaries :: [float()] | match_spec([float()]),
-         record_min_max :: boolean() | match_spec(boolean()),
-         checkpoint :: #explicit_histogram_checkpoint{} | match_spec(#explicit_histogram_checkpoint{}) | {#explicit_histogram_checkpoint{}},
+         explicit_bucket_boundaries :: match_spec([float()]) | undefined,
+         record_min_max :: match_spec(boolean()) | undefined,
+         checkpoint :: match_spec(#explicit_histogram_checkpoint{}) | {#explicit_histogram_checkpoint{}} | undefined,
          bucket_counts :: counters:counters_ref() | match_spec(undefined),
-         min :: number() | infinity | match_spec(number()),
-         max :: number() | match_spec(number()),
-         sum :: number() | match_spec(number())
+         min :: infinity | match_spec(number()) | undefined,
+         max :: match_spec(number()) | undefined,
+         sum :: match_spec(number()) | undefined
         }).
 
 -record(datapoint,
@@ -96,16 +97,16 @@
 -record(histogram_datapoint,
         {
          attributes :: opentelemetry:attributes_map(),
-         start_time_unix_nano :: integer() | match_spec(integer()) | {const, eqwalizer:dynamic()},
+         start_time_unix_nano :: match_spec(integer()) | {const, eqwalizer:dynamic()}  | undefined,
          time_unix_nano :: integer(),
          count :: number(),
-         sum :: float() | match_spec(integer()),
+         sum :: float() | match_spec(integer()) | undefined,
          bucket_counts :: list(),
-         explicit_bounds :: [float()] | match_spec([float()]),
+         explicit_bounds :: match_spec([float()]) | undefined,
          exemplars :: list(),
          flags :: integer(),
-         min :: integer() | infinity | match_spec(integer()),
-         max :: integer() | match_spec(integer())
+         min ::  infinity | match_spec(integer()) | undefined,
+         max :: match_spec(integer()) | undefined
         }).
 
 -record(histogram,
diff --git a/apps/opentelemetry_experimental/src/otel_aggregation.erl b/apps/opentelemetry_experimental/src/otel_aggregation.erl
@@ -12,13 +12,11 @@
              otel_aggregation_last_value:t() | otel_aggregation_histogram_explicit:t().
 
 -type key() :: {atom(), opentelemetry:attributes_map(), reference()}.
--type key_match_spec() :: match_spec(otel_aggregation:key()) | {match_spec(atom()), match_spec(opentelemetry:attributes_map()), match_spec(reference())}.
 
 -type options() :: map().
 
 -export_type([t/0,
               key/0,
-              key_match_spec/0,
               options/0]).
 
 %% Returns the aggregation's record as it is seen and updated by
diff --git a/apps/opentelemetry_experimental/src/otel_aggregation_histogram_explicit.erl b/apps/opentelemetry_experimental/src/otel_aggregation_histogram_explicit.erl
@@ -175,7 +175,6 @@ aggregate(Table, #view_aggregation{name=Name,
             false
     end.
 
--dialyzer({nowarn_function, checkpoint/3}).
 checkpoint(Tab, #view_aggregation{name=Name,
                                   reader=ReaderId,
                                   temporality=?TEMPORALITY_DELTA}, CollectionStartNano) ->
diff --git a/apps/opentelemetry_experimental/src/otel_aggregation_last_value.erl b/apps/opentelemetry_experimental/src/otel_aggregation_last_value.erl
@@ -53,7 +53,6 @@ aggregate(Tab, ViewAggregation=#view_aggregation{name=Name,
             ets:insert(Tab, ?assert_type((?assert_type(Metric, #last_value_aggregation{}))#last_value_aggregation{value=Value}, tuple()))
     end.
 
--dialyzer({nowarn_function, checkpoint/3}).
 checkpoint(Tab, #view_aggregation{name=Name,
                                   reader=ReaderId,
                                   temporality=?TEMPORALITY_DELTA}, CollectionStartNano) ->
diff --git a/apps/opentelemetry_experimental/src/otel_aggregation_sum.erl b/apps/opentelemetry_experimental/src/otel_aggregation_sum.erl
@@ -89,7 +89,6 @@ aggregate(_Tab, #view_aggregation{name=_Name,
                                   is_monotonic=_IsMonotonic}, _Value, _) ->
     false.
 
--dialyzer({nowarn_function, checkpoint/3}).
 checkpoint(Tab, #view_aggregation{name=Name,
                                   reader=ReaderId,
                                   temporality=?TEMPORALITY_DELTA}, CollectionStartNano) ->
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,15 @@ code_change(State) ->
 
 %%
 
+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, named_wildcard_view} ->
+            {reply, false, State}
+    end.
+
 instruments_tab(Name) ->
     ets:new(list_to_atom(lists:concat([instruments, "_", Name])), [set,
                                                                    named_table,
@@ -282,19 +286,21 @@ metrics_tab(Name) ->
                                                                {keypos, 2},
                                                                public]).
 
--dialyzer({nowarn_function,new_view/1}).
 new_view(ViewConfig) ->
     Name = maps:get(name, ViewConfig, undefined),
     Description = maps:get(description, ViewConfig, undefined),
     Selector = maps:get(selector, ViewConfig, undefined),
     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, named_wildcard_view} -> 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_meter_server_sup.erl b/apps/opentelemetry_experimental/src/otel_meter_server_sup.erl
@@ -29,7 +29,6 @@
 start_link(Name, Resource, Opts) ->
     supervisor:start_link(?MODULE, [Name, Resource, Opts]).
 
--dialyzer({nowarn_function, provider_pid/1}).
 -spec provider_pid(supervisor:sup_ref()) -> pid() | restarting | undefined.
 provider_pid(SupPid) ->
     Children = supervisor:which_children(SupPid),
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,24 @@
 
 -include_lib("opentelemetry_api/include/gradualizer.hrl").
 
+
+-spec new(criteria() | undefined, config()) -> {ok, t()} | {error, named_wildcard_view}.
+new(Criteria, Config) ->
+    new(undefined, Criteria, Config).
+
+-spec new(name(), criteria() | undefined, config()) -> {ok, t()} | {error, named_wildcard_view}.
+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, named_wildcard_view};
+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,new/2}).
--spec new(criteria() | undefined, config()) -> t().
-new(Criteria, Config) ->
+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,15 +80,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,
                                                  meter=Meter,
@@ -137,11 +142,12 @@ value_or(undefined, Other) ->
 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};
@@ -166,24 +172,23 @@ criteria_to_instrument_matchspec(_) ->
     %% eqwalizer:ignore building a matchspec and don't want '_' polluting the type
     ets:match_spec_compile([{#instrument{_='_'}, [], [true]}]).
 
--dialyzer({nowarn_function,maybe_init_meter/1}).
 maybe_init_meter(#instrument{meter='_'}) ->
     {'_', #meter{instrumentation_scope=#instrumentation_scope{_='_'},
                  _='_'}}.
 
--dialyzer({nowarn_function,update_meter_name/2}).
 update_meter_name(MeterName, {_, Meter=#meter{instrumentation_scope=Scope}}) ->
     {'_', Meter#meter{instrumentation_scope=Scope#instrumentation_scope{name=MeterName}}}.
 
--dialyzer({nowarn_function,update_meter_version/2}).
 update_meter_version(MeterVersion, {_, Meter=#meter{instrumentation_scope=Scope}}) ->
     {'_', Meter#meter{instrumentation_scope=Scope#instrumentation_scope{version=MeterVersion}}}.
 
--dialyzer({nowarn_function,update_meter_schema_url/2}).
 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/src/otel_view.hrl b/apps/opentelemetry_experimental/src/otel_view.hrl
@@ -11,7 +11,7 @@
          name :: atom(),
          scope :: opentelemetry:instrumentation_scope(),
          instrument :: otel_instrument:t(),
-         reader :: reference(),
+         reader :: reference() | undefined,
 
          attribute_keys :: [opentelemetry:attribute_key()] | undefined,
 
diff --git a/apps/opentelemetry_experimental/test/otel_metrics_SUITE.erl b/apps/opentelemetry_experimental/test/otel_metrics_SUITE.erl
