Merge pull request #632 from albertored/spec-compliance

tsloughter · web-flow · commit cbce85f64be2 · 2023-11-22T03:30:37.000-07:00
Improve spec compliance
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -36,6 +36,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - [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)
+- [Attributes are optional in Counter.add(), UpDownCounter.add() and Histo.record()](https://github.com/open-telemetry/opentelemetry-erlang/pull/632)
 
 ## Experimental SDK
 
@@ -55,6 +56,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
  - [Correctly record histogram values greater than last boundary](https://github.com/open-telemetry/opentelemetry-erlang/pull/614)
  - [Readers should use a default cumulative temporality if not specified](https://github.com/open-telemetry/opentelemetry-erlang/pull/613)
+ - [Check for positive data values in counters and histograms](https://github.com/open-telemetry/opentelemetry-erlang/pull/632)
 
 ## SDK 1.3.1 - 2023-08-15
 
diff --git a/apps/opentelemetry_api_experimental/include/otel_meter.hrl b/apps/opentelemetry_api_experimental/include/otel_meter.hrl
@@ -25,12 +25,21 @@
 -define(create_observable_updowncounter(Name, Callback, CallbackArgs, Opts),
         otel_meter:create_observable_updowncounter(?current_meter, Name, Callback, CallbackArgs, Opts)).
 
+-define(counter_add(Name, Number),
+        otel_counter:add(?current_meter, Name, Number)).
+
 -define(counter_add(Name, Number, Attributes),
         otel_counter:add(?current_meter, Name, Number, Attributes)).
 
+-define(updown_counter_add(Name, Number),
+        otel_updown_counter:add(?current_meter, Name, Number)).
+
 -define(updown_counter_add(Name, Number, Attributes),
         otel_updown_counter:add(?current_meter, Name, Number, Attributes)).
 
+-define(histogram_record(Name, Number),
+        otel_histogram:record(?current_meter, Name, Number)).
+
 -define(histogram_record(Name, Number, Attributes),
         otel_histogram:record(?current_meter, Name, Number, Attributes)).
 
diff --git a/apps/opentelemetry_api_experimental/lib/open_telemetry/counter.ex b/apps/opentelemetry_api_experimental/lib/open_telemetry/counter.ex
@@ -13,6 +13,16 @@ defmodule OpenTelemetryAPIExperimental.Counter do
     end
   end
 
+  defmacro add(name, number) do
+    quote bind_quoted: [name: name, number: number] do
+      :otel_counter.add(
+        :opentelemetry_experimental.get_meter(:opentelemetry.get_application_scope(__MODULE__)),
+        name,
+        number
+      )
+    end
+  end
+
   defmacro add(name, number, attributes) do
     quote bind_quoted: [name: name, number: number, attributes: attributes] do
       :otel_counter.add(
diff --git a/apps/opentelemetry_api_experimental/lib/open_telemetry/histogram.ex b/apps/opentelemetry_api_experimental/lib/open_telemetry/histogram.ex
@@ -13,6 +13,16 @@ defmodule OpenTelemetryAPIExperimental.Histogram do
     end
   end
 
+  defmacro record(name, number) do
+    quote bind_quoted: [name: name, number: number] do
+      :otel_histogram.record(
+        :opentelemetry_experimental.get_meter(:opentelemetry.get_application_scope(__MODULE__)),
+        name,
+        number
+      )
+    end
+  end
+
   defmacro record(name, number, attributes) do
     quote bind_quoted: [name: name, number: number, attributes: attributes] do
       :otel_histogram.record(
diff --git a/apps/opentelemetry_api_experimental/lib/open_telemetry/updown_counter.ex b/apps/opentelemetry_api_experimental/lib/open_telemetry/updown_counter.ex
@@ -13,6 +13,16 @@ defmodule OpenTelemetryAPIExperimental.UpDownCounter do
     end
   end
 
+  defmacro add(name, number) do
+    quote bind_quoted: [name: name, number: number] do
+      :otel_updown_counter.add(
+        :opentelemetry_experimental.get_meter(:opentelemetry.get_application_scope(__MODULE__)),
+        name,
+        number
+      )
+    end
+  end
+
   defmacro add(name, number, attributes) do
     quote bind_quoted: [name: name, number: number, attributes: attributes] do
       :otel_updown_counter.add(
diff --git a/apps/opentelemetry_api_experimental/src/otel_counter.erl b/apps/opentelemetry_api_experimental/src/otel_counter.erl
@@ -19,6 +19,7 @@
 -module(otel_counter).
 
 -export([create/3,
+         add/2,
          add/3,
          add/4]).
 
@@ -32,10 +33,20 @@
 create(Meter, Name, Opts) ->
     otel_meter:create_counter(Meter, Name, Opts).
 
--spec add(otel_meter:t(), otel_instrument:name(), pos_integer() |float(), opentelemetry:attributes_map()) -> ok.
-add(Meter, Name, Number, Attributes) ->
-    otel_meter:record(Meter, Name, Number, Attributes).
+-spec add(otel_instrument:t(), pos_integer() | float()) -> ok.
+add(Instrument=#instrument{module=Module}, Number) ->
+    Module:record(Instrument, Number).
 
--spec add(otel_instrument:t(), pos_integer() |float(), opentelemetry:attributes_map()) -> ok.
+-spec add(
+    otel_meter:t() | otel_instrument:t(),
+    otel_instrument:name() | pos_integer() | float(),
+    pos_integer() | float() | opentelemetry:attributes_map()) -> ok.
 add(Instrument=#instrument{module=Module}, Number, Attributes) ->
-    Module:record(Instrument, Number, Attributes).
+    Module:record(Instrument, Number, Attributes);
+
+add(Meter, Name, Number) ->
+    otel_meter:record(Meter, Name, Number).
+
+-spec add(otel_meter:t(), otel_instrument:name(), pos_integer() |float(), opentelemetry:attributes_map()) -> ok.
+add(Meter, Name, Number, Attributes) ->
+    otel_meter:record(Meter, Name, Number, Attributes).
diff --git a/apps/opentelemetry_api_experimental/src/otel_histogram.erl b/apps/opentelemetry_api_experimental/src/otel_histogram.erl
@@ -20,6 +20,7 @@
 -module(otel_histogram).
 
 -export([create/3,
+         record/2,
          record/3,
          record/4]).
 
@@ -33,10 +34,20 @@
 create(Meter, Name, Opts) ->
     otel_meter:create_histogram(Meter, Name, Opts).
 
--spec record(otel_meter:t(), otel_instrument:name(), pos_integer() | float(), opentelemetry:attributes_map()) -> ok.
-record(Meter, Name, Number, Attributes) ->
-    otel_meter:record(Meter, Name, Number, Attributes).
+-spec record(otel_instrument:t(), pos_integer() | float()) -> ok.
+record(Instrument=#instrument{module=Module}, Number) ->
+    Module:record(Instrument, Number).
 
--spec record(otel_instrument:t(), pos_integer() | float(), opentelemetry:attributes_map()) -> ok.
+-spec record(
+    otel_meter:t() | otel_instrument:t(),
+    otel_instrument:name() | pos_integer() | float(),
+    pos_integer() | float() | opentelemetry:attributes_map()) -> ok.
 record(Instrument=#instrument{module=Module}, Number, Attributes) ->
-    Module:record(Instrument, Number, Attributes).
+    Module:record(Instrument, Number, Attributes);
+
+record(Meter, Name, Number) ->
+    otel_meter:record(Meter, Name, Number).
+
+-spec record(otel_meter:t(), otel_instrument:name(), pos_integer() | float(), opentelemetry:attributes_map()) -> ok.
+record(Meter, Name, Number, Attributes) ->
+    otel_meter:record(Meter, Name, Number, Attributes).
diff --git a/apps/opentelemetry_api_experimental/src/otel_meter.erl b/apps/opentelemetry_api_experimental/src/otel_meter.erl
@@ -34,6 +34,7 @@
 
          lookup_instrument/2,
 
+         record/3,
          record/4]).
 
 -include("otel_metrics.hrl").
@@ -169,5 +170,8 @@ lookup_instrument(Meter={Module, _}, Name) ->
 register_callback(Meter={Module, _}, Instruments, Callback, CallbackArgs) ->
     Module:register_callback(Meter, Instruments, Callback, CallbackArgs).
 
+record(Meter={Module, _}, Name, Number) ->
+    Module:record(Meter, Name, Number).
+
 record(Meter={Module, _}, Name, Number, Attributes) ->
     Module:record(Meter, Name, Number, Attributes).
diff --git a/apps/opentelemetry_api_experimental/src/otel_updown_counter.erl b/apps/opentelemetry_api_experimental/src/otel_updown_counter.erl
@@ -19,6 +19,7 @@
 -module(otel_updown_counter).
 
 -export([create/3,
+         add/2,
          add/3,
          add/4]).
 
@@ -32,10 +33,20 @@
 create(Meter, Name, Opts) ->
     otel_meter:create_updown_counter(Meter, Name, Opts).
 
--spec add(otel_meter:t(), otel_instrument:name(), number(), opentelemetry:attributes_map()) -> ok.
-add(Meter, Name, Number, Attributes) ->
-    otel_meter:record(Meter, Name, Number, Attributes).
+-spec add(otel_instrument:t(), number()) -> ok.
+add(Instrument=#instrument{module=Module}, Number) ->
+    Module:record(Instrument, Number).
 
--spec add(otel_instrument:t(), number(), opentelemetry:attributes_map()) -> ok.
+-spec add(
+    otel_meter:t() | otel_instrument:t(),
+    otel_instrument:name() | number(),
+    number() | opentelemetry:attributes_map()) -> ok.
 add(Instrument=#instrument{module=Module}, Number, Attributes) ->
-    Module:record(Instrument, Number, Attributes).
+    Module:record(Instrument, Number, Attributes);
+
+add(Meter, Name, Number) ->
+    otel_meter:record(Meter, Name, Number).
+
+-spec add(otel_meter:t(), otel_instrument:name(), number(), opentelemetry:attributes_map()) -> ok.
+add(Meter, Name, Number, Attributes) ->
+    otel_meter:record(Meter, Name, Number, Attributes).
diff --git a/apps/opentelemetry_experimental/src/otel_aggregation_sum.erl b/apps/opentelemetry_experimental/src/otel_aggregation_sum.erl
@@ -44,10 +44,8 @@ init(#view_aggregation{name=Name,
                      float_value=0.0}.
 
 aggregate(Tab, #view_aggregation{name=Name,
-                                 reader=ReaderId,
-                                 is_monotonic=IsMonotonic}, Value, Attributes)
-  when is_integer(Value) andalso
-       ((IsMonotonic andalso Value >= 0) orelse not IsMonotonic) ->
+                                 reader=ReaderId}, Value, Attributes)
+  when is_integer(Value) ->
     Key = {Name, Attributes, ReaderId},
     try
         _ = ets:update_counter(Tab, Key, {#sum_aggregation.int_value, Value}),
@@ -65,9 +63,7 @@ aggregate(Tab, #view_aggregation{name=Name,
             false
     end;
 aggregate(Tab, #view_aggregation{name=Name,
-                                 reader=ReaderId,
-                                 is_monotonic=IsMonotonic}, Value, Attributes)
-  when (IsMonotonic andalso Value >= 0.0) orelse not IsMonotonic ->
+                                 reader=ReaderId}, Value, Attributes) ->
     Key = {Name, Attributes, ReaderId},
     MS = [{#sum_aggregation{key=Key,
                             start_time='$1',
@@ -84,10 +80,7 @@ aggregate(Tab, #view_aggregation{name=Name,
                               previous_checkpoint='$6',
                               int_value='$3',
                               float_value={'+', '$4', {const, Value}}}}]}],
-    1 =:= ets:select_replace(Tab, MS);
-aggregate(_Tab, #view_aggregation{name=_Name,
-                                  is_monotonic=_IsMonotonic}, _Value, _) ->
-    false.
+    1 =:= ets:select_replace(Tab, MS).
 
 checkpoint(Tab, #view_aggregation{name=Name,
                                   reader=ReaderId,
diff --git a/apps/opentelemetry_experimental/src/otel_meter_default.erl b/apps/opentelemetry_experimental/src/otel_meter_default.erl
@@ -25,7 +25,8 @@
          register_callback/4,
          scope/1]).
 
--export([record/3,
+-export([record/2,
+         record/3,
          record/4]).
 
 -include_lib("kernel/include/logger.hrl").
@@ -111,6 +112,13 @@ validate_explicit_bucket_boundaries(Name, Value) ->
     false.
 %%
 
+
+record(Instrument=#instrument{}, Number) ->
+    record(Instrument, Number, #{}).
+
+record(Meter={_,#meter{}}, Name, Number) ->
+    record(Meter, Name, Number, #{});
+
 record(Instrument=#instrument{meter={_, #meter{view_aggregations_tab=ViewAggregationTab,
                                                metrics_tab=MetricsTab}}}, Number, Attributes) ->
     otel_meter_server:record(ViewAggregationTab, MetricsTab, Instrument, Number, Attributes).
diff --git a/apps/opentelemetry_experimental/src/otel_meter_server.erl b/apps/opentelemetry_experimental/src/otel_meter_server.erl
@@ -374,15 +374,20 @@ handle_measurement(Meter, Name, Number, Attributes, ViewAggregationsTab, Metrics
     update_aggregations(Number, Attributes, Matches, MetricsTab).
 
 update_aggregations(Value, Attributes, ViewAggregations, MetricsTab) ->
-    lists:foreach(fun([ViewAggregation=#view_aggregation{}]) ->
-                          otel_aggregation:maybe_init_aggregate(MetricsTab,
-                                                                ViewAggregation,
-                                                                Value,
-                                                                Attributes);
+    lists:foreach(fun([ViewAggregation=#view_aggregation{instrument=Instrument}]) ->
+                        maybe_init_aggregate(Value, Instrument, MetricsTab, ViewAggregation, Attributes);
                      (_) ->
                           ok
                   end, ViewAggregations).
 
+maybe_init_aggregate(Value, #instrument{kind=Kind} = Instrument, _MetricsTab, _ViewAggregation, _Attributes)
+        when Value < 0, Kind == ?KIND_COUNTER orelse Kind == ?KIND_HISTOGRAM ->
+    ?LOG_INFO("Discarding negative value for instrument ~s of type ~s", [Instrument#instrument.name, Kind]),
+    ok;
+
+maybe_init_aggregate(Value, _Instrument, MetricsTab, ViewAggregation, Attributes) ->
+    otel_aggregation:maybe_init_aggregate(MetricsTab, ViewAggregation, Value, Attributes).
+
 %% create an aggregation for each Reader and its possibly unique aggregation/temporality
 per_reader_aggregations(Reader, Instrument, ViewAggregations) ->
     [view_aggregation_for_reader(Instrument, ViewAggregation, View, Reader)
diff --git a/apps/opentelemetry_experimental/test/otel_metrics_SUITE.erl b/apps/opentelemetry_experimental/test/otel_metrics_SUITE.erl
@@ -263,10 +263,18 @@ float_counter(_Config) ->
     ?assertEqual(ok, ?counter_add(CounterName, 5.5, #{<<"c">> => <<"b">>})),
     ?assertEqual(ok, ?counter_add(CounterName, 5, #{<<"c">> => <<"b">>})),
 
+     %% without attributes
+    ?assertEqual(ok, ?counter_add(CounterName, 1.2)),
+    ?assertEqual(ok, otel_counter:add(Counter, 2.1)),
+
+    %% negative values are discarded
+    ?assertEqual(ok, ?counter_add(CounterName, -2, #{<<"c">> => <<"b">>})),
+    ?assertEqual(ok, otel_counter:add(Counter, -2)),
+
     otel_meter_server:force_flush(),
 
     ?assertSumReceive(f_counter, <<"macro made counter description">>, kb,
-                      [{20.8, #{<<"c">> => <<"b">>}}]),
+                      [{3.3, #{}}, {20.8, #{<<"c">> => <<"b">>}}]),
 
     ok.
 
@@ -287,10 +295,15 @@ float_updown_counter(_Config) ->
     ?assertEqual(ok, ?updown_counter_add(CounterName, -5.5, #{<<"c">> => <<"b">>})),
     ?assertEqual(ok, ?updown_counter_add(CounterName, 5, #{<<"c">> => <<"b">>})),
 
+    %% without attributes
+    ?assertEqual(ok, ?updown_counter_add(CounterName, 1.2)),
+    ?assertEqual(ok, otel_updown_counter:add(Counter, 2.1)),
+
+
     otel_meter_server:force_flush(),
 
     ?assertSumReceive(f_counter, <<"macro made updown counter description">>, kb,
-                      [{10.0, #{<<"c">> => <<"b">>}}]),
+                      [{3.3, #{}}, {10.0, #{<<"c">> => <<"b">>}}]),
 
     ok.
 
@@ -310,6 +323,14 @@ float_histogram(_Config) ->
     ?assertEqual(ok, otel_histogram:record(Counter, 10.3, #{<<"c">> => <<"b">>})),
     ?assertEqual(ok, otel_histogram:record(Counter, 10.3, #{<<"c">> => <<"b">>})),
     ?assertEqual(ok, ?histogram_record(CounterName, 5.5, #{<<"c">> => <<"b">>})),
+    
+    %% without attributes
+    ?assertEqual(ok, ?histogram_record(CounterName, 1.2)),
+    ?assertEqual(ok, otel_histogram:record(Counter, 2.1)),
+
+    %% negative values are discarded
+    ?assertEqual(ok, ?histogram_record(CounterName, -2, #{<<"c">> => <<"b">>})),
+    ?assertEqual(ok, otel_histogram:record(Counter, -2)),
 
     %% float type accepts integers
     ?assertEqual(ok, ?histogram_record(CounterName, 5, #{<<"c">> => <<"b">>})),
@@ -326,7 +347,9 @@ float_histogram(_Config) ->
                                          min=Min,
                                          max=Max,
                                          sum=Sum}  <- Datapoints],
-            ?assertEqual([], [{#{<<"c">> => <<"b">>}, [0,1,1,2,0,0,0,0,0,0,0,0,0,0,0,0], 5, 10.3, 31.1}]
+            ?assertEqual([], [
+                              {#{<<"c">> => <<"b">>}, [0,1,1,2,0,0,0,0,0,0,0,0,0,0,0,0], 5, 10.3, 31.1},
+                              {#{}, [0,2,0,0,0,0,0,0,0,0,0,0,0,0,0,0], 1.2, 2.1, 3.3}]
                          -- AttributeBuckets, AttributeBuckets)
     after
         5000 ->
