From 96ef35d8b3199ac00167f1771b7148916ea11207 Mon Sep 17 00:00:00 2001 From: Alberto Sartori Date: Tue, 3 Oct 2023 17:04:41 +0200 Subject: [PATCH 1/5] attributes optional in counter.add and histo.record function --- .../include/otel_meter.hrl | 9 ++++++++ .../lib/open_telemetry/counter.ex | 10 +++++++++ .../lib/open_telemetry/histogram.ex | 10 +++++++++ .../lib/open_telemetry/updown_counter.ex | 10 +++++++++ .../src/otel_counter.erl | 21 ++++++++++++++----- .../src/otel_histogram.erl | 21 ++++++++++++++----- .../src/otel_meter.erl | 4 ++++ .../src/otel_updown_counter.erl | 21 ++++++++++++++----- .../src/otel_meter_default.erl | 10 ++++++++- .../test/otel_metrics_SUITE.erl | 21 ++++++++++++++++--- 10 files changed, 118 insertions(+), 19 deletions(-) diff --git a/apps/opentelemetry_api_experimental/include/otel_meter.hrl b/apps/opentelemetry_api_experimental/include/otel_meter.hrl index f25417d8..e983f75e 100644 --- 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 index 9955ecf9..aae952dd 100644 --- 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 index e371150e..929798d7 100644 --- 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 index 6b424668..4e10ee3a 100644 --- 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 index 36dd2257..88777995 100644 --- 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). \ No newline at end of file diff --git a/apps/opentelemetry_api_experimental/src/otel_histogram.erl b/apps/opentelemetry_api_experimental/src/otel_histogram.erl index e16eef14..4dc51de1 100644 --- 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). \ No newline at end of file diff --git a/apps/opentelemetry_api_experimental/src/otel_meter.erl b/apps/opentelemetry_api_experimental/src/otel_meter.erl index 2b9cb9dc..f82fcfbe 100644 --- 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). \ No newline at end of file diff --git a/apps/opentelemetry_api_experimental/src/otel_updown_counter.erl b/apps/opentelemetry_api_experimental/src/otel_updown_counter.erl index 64c73d0a..72063057 100644 --- 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). \ No newline at end of file diff --git a/apps/opentelemetry_experimental/src/otel_meter_default.erl b/apps/opentelemetry_experimental/src/otel_meter_default.erl index 656aaeda..06c0f011 100644 --- 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/test/otel_metrics_SUITE.erl b/apps/opentelemetry_experimental/test/otel_metrics_SUITE.erl index 47bf87a8..489511da 100644 --- a/apps/opentelemetry_experimental/test/otel_metrics_SUITE.erl +++ b/apps/opentelemetry_experimental/test/otel_metrics_SUITE.erl @@ -263,10 +263,14 @@ 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)), + 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 +291,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 +319,10 @@ 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)), %% float type accepts integers ?assertEqual(ok, ?histogram_record(CounterName, 5, #{<<"c">> => <<"b">>})), @@ -326,7 +339,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 -> From 4027fc04e2eaa36b9d1fcea1202ace04895a1ee8 Mon Sep 17 00:00:00 2001 From: Alberto Sartori Date: Tue, 3 Oct 2023 18:10:10 +0200 Subject: [PATCH 2/5] counter.add and histo.record should accept only positive numbers --- .../src/otel_meter_server.erl | 15 ++++++++++----- .../test/otel_metrics_SUITE.erl | 8 ++++++++ 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/apps/opentelemetry_experimental/src/otel_meter_server.erl b/apps/opentelemetry_experimental/src/otel_meter_server.erl index 55e2f762..a16c87de 100644 --- 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 index 489511da..4dd32fac 100644 --- a/apps/opentelemetry_experimental/test/otel_metrics_SUITE.erl +++ b/apps/opentelemetry_experimental/test/otel_metrics_SUITE.erl @@ -267,6 +267,10 @@ float_counter(_Config) -> ?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, @@ -324,6 +328,10 @@ float_histogram(_Config) -> ?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">>})), From 0e5ae107bf47510879627e2095ac3800c89810a9 Mon Sep 17 00:00:00 2001 From: Alberto Sartori Date: Tue, 3 Oct 2023 18:15:26 +0200 Subject: [PATCH 3/5] Changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b6f43355..c3ad880a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,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 @@ -50,6 +51,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) + - [Counter.add() and Histogram.record() accept only positive numbers](https://github.com/open-telemetry/opentelemetry-erlang/pull/632) ## SDK 1.3.1 - 2023-08-15 From 9e02f4b67fe33b7d951944089f65715eca4795b5 Mon Sep 17 00:00:00 2001 From: Alberto Sartori Date: Sun, 5 Nov 2023 14:37:29 +0100 Subject: [PATCH 4/5] Update changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c3ad880a..320c2095 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,7 +51,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) - - [Counter.add() and Histogram.record() accept only positive numbers](https://github.com/open-telemetry/opentelemetry-erlang/pull/632) + - [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 From 44bc2a16a76470b79b205e40f7881f264433d029 Mon Sep 17 00:00:00 2001 From: Alberto Sartori Date: Sun, 5 Nov 2023 14:39:22 +0100 Subject: [PATCH 5/5] No more needed to check for pos numbers in sum aggregation --- .../src/otel_aggregation_sum.erl | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/apps/opentelemetry_experimental/src/otel_aggregation_sum.erl b/apps/opentelemetry_experimental/src/otel_aggregation_sum.erl index 2bcf9b6b..c57596e4 100644 --- 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,