From 0bdac8e6b075a7582a624230f1cf00a8bca03155 Mon Sep 17 00:00:00 2001 From: Alberto Sartori Date: Tue, 19 Sep 2023 08:22:49 +0000 Subject: [PATCH 1/7] Add advisory_params option to instruments --- .../include/otel_metrics.hrl | 19 +++--- .../src/otel_counter.erl | 2 +- .../src/otel_histogram.erl | 2 +- .../src/otel_instrument.erl | 62 ++++++++++++------- .../src/otel_meter.erl | 32 +++++----- .../src/otel_meter_noop.erl | 6 +- .../src/otel_updown_counter.erl | 2 +- .../src/otel_meter_default.erl | 10 ++- 8 files changed, 72 insertions(+), 63 deletions(-) diff --git a/apps/opentelemetry_api_experimental/include/otel_metrics.hrl b/apps/opentelemetry_api_experimental/include/otel_metrics.hrl index 5446cf2e..75716b1b 100644 --- a/apps/opentelemetry_api_experimental/include/otel_metrics.hrl +++ b/apps/opentelemetry_api_experimental/include/otel_metrics.hrl @@ -1,12 +1,13 @@ --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}). +-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}). -define(TEMPORALITY_DELTA, temporality_delta). -define(TEMPORALITY_CUMULATIVE, temporality_cumulative). diff --git a/apps/opentelemetry_api_experimental/src/otel_counter.erl b/apps/opentelemetry_api_experimental/src/otel_counter.erl index 3c0e9728..36dd2257 100644 --- a/apps/opentelemetry_api_experimental/src/otel_counter.erl +++ b/apps/opentelemetry_api_experimental/src/otel_counter.erl @@ -28,7 +28,7 @@ -spec create(Meter, Name, Opts) -> otel_instrument:t() when Meter :: otel_meter:t(), Name :: otel_instrument:name(), - Opts :: otel_meter:opts(). + Opts :: otel_instrument:opts(). create(Meter, Name, Opts) -> otel_meter:create_counter(Meter, Name, Opts). diff --git a/apps/opentelemetry_api_experimental/src/otel_histogram.erl b/apps/opentelemetry_api_experimental/src/otel_histogram.erl index f82a55ea..e16eef14 100644 --- a/apps/opentelemetry_api_experimental/src/otel_histogram.erl +++ b/apps/opentelemetry_api_experimental/src/otel_histogram.erl @@ -29,7 +29,7 @@ -spec create(Meter, Name, Opts) -> otel_instrument:t() when Meter :: otel_meter:t(), Name :: otel_instrument:name(), - Opts :: otel_meter:opts(). + Opts :: otel_instrument:opts(). create(Meter, Name, Opts) -> otel_meter:create_histogram(Meter, Name, Opts). diff --git a/apps/opentelemetry_api_experimental/src/otel_instrument.erl b/apps/opentelemetry_api_experimental/src/otel_instrument.erl index 3d37b4f7..c728ffe1 100644 --- a/apps/opentelemetry_api_experimental/src/otel_instrument.erl +++ b/apps/opentelemetry_api_experimental/src/otel_instrument.erl @@ -17,8 +17,8 @@ %%%------------------------------------------------------------------------- -module(otel_instrument). --export([new/6, - new/8, +-export([new/5, + new/7, is_monotonic/1, temporality/1]). @@ -38,6 +38,12 @@ -type temporality() :: ?TEMPORALITY_DELTA | ?TEMPORALITY_CUMULATIVE. +-type advisory_params() :: #{explicit_bucket_boundaries => [number(), ...]}. + +-type opts() :: #{description => otel_instrument:description(), + unit => otel_instrument:unit(), + advisory_params => otel_instrument:advisory_params()}. + -type t() :: #instrument{}. -export_type([t/0, @@ -48,29 +54,39 @@ temporality/0, callback/0, callback_args/0, - callback_result/0]). + callback_result/0, + advisory_params/0, + opts/0]). --spec new(module(), otel_meter:t(), kind(), name(), description() | undefined, unit() | undefined) -> t(). -new(Module, Meter, Kind, Name, Description, Unit) -> - #instrument{module = Module, - meter = Meter, - name = Name, - description = Description, - temporality = ?TEMPORALITY_DELTA, - kind = Kind, - unit = Unit}. +-spec new(module(), otel_meter:t(), kind(), name(), opts()) -> t(). +new(Module, Meter, Kind, Name, Opts) -> + Description = maps:get(description, Opts, undefined), + Unit = maps:get(unit, Opts, undefined), + AdvisoryParams = maps:get(advisory_params, Opts, undefined), + #instrument{module = Module, + meter = Meter, + name = Name, + description = Description, + temporality = ?TEMPORALITY_DELTA, + kind = Kind, + unit = Unit, + advisory_params = AdvisoryParams}. --spec new(module(), otel_meter:t(), kind(), name(), description() | undefined, unit() | undefined, callback(), callback_args()) -> t(). -new(Module, Meter, Kind, Name, Description, Unit, Callback, CallbackArgs) -> - #instrument{module = Module, - meter = Meter, - name = Name, - description = Description, - kind = Kind, - unit = Unit, - temporality = ?TEMPORALITY_CUMULATIVE, - callback = Callback, - callback_args = CallbackArgs}. +-spec new(module(), otel_meter:t(), kind(), name(), callback(), callback_args(), opts()) -> t(). +new(Module, Meter, Kind, Name, Callback, CallbackArgs, Opts) -> + Description = maps:get(description, Opts, undefined), + Unit = maps:get(unit, Opts, undefined), + AdvisoryParams = maps:get(advisory_params, Opts, undefined), + #instrument{module = Module, + meter = Meter, + name = Name, + description = Description, + kind = Kind, + unit = Unit, + temporality = ?TEMPORALITY_CUMULATIVE, + callback = Callback, + callback_args = CallbackArgs, + advisory_params = AdvisoryParams}. is_monotonic(#instrument{kind=?KIND_COUNTER}) -> true; diff --git a/apps/opentelemetry_api_experimental/src/otel_meter.erl b/apps/opentelemetry_api_experimental/src/otel_meter.erl index fbda6063..2b9cb9dc 100644 --- a/apps/opentelemetry_api_experimental/src/otel_meter.erl +++ b/apps/opentelemetry_api_experimental/src/otel_meter.erl @@ -42,7 +42,7 @@ Meter :: t(), Name :: otel_instrument:name(), Kind :: otel_instrument:kind(), - Opts :: opts(). + Opts :: otel_instrument:opts(). -callback create_instrument(Meter, Name, Kind, Callback, CallbackArgs, Opts) -> otel_instrument:t() when Meter :: t(), @@ -50,7 +50,7 @@ Kind :: otel_instrument:kind(), Callback :: otel_instrument:callback(), CallbackArgs :: otel_instrument:callback_args(), - Opts :: opts(). + Opts :: otel_instrument:opts(). -callback register_callback(Meter, Instruments, Callback, CallbackArgs) -> ok when Meter :: t(), @@ -58,39 +58,35 @@ Callback :: otel_instrument:callback(), CallbackArgs :: otel_instrument:callback_args(). --type opts() :: #{description => otel_instrument:description(), - unit => otel_instrument:unit()}. - -type t() :: {module(), term()}. --export_type([t/0, - opts/0]). +-export_type([t/0]). -spec create_counter(Meter, Name, Opts) -> otel_instrument:t() when Meter :: t(), Name :: otel_instrument:name(), - Opts :: opts(). + Opts :: otel_instrument:opts(). create_counter(Meter, Name, Opts) -> create_instrument(Meter, Name, ?KIND_COUNTER, Opts). -spec create_updown_counter(Meter, Name, Opts) -> otel_instrument:t() when Meter :: t(), Name :: otel_instrument:name(), - Opts :: opts(). + Opts :: otel_instrument:opts(). create_updown_counter(Meter, Name, Opts) -> create_instrument(Meter, Name, ?KIND_UPDOWN_COUNTER, Opts). -spec create_histogram(Meter, Name, Opts) -> otel_instrument:t() when Meter :: t(), Name :: otel_instrument:name(), - Opts :: opts(). + Opts :: otel_instrument:opts(). create_histogram(Meter, Name, Opts) -> create_instrument(Meter, Name, ?KIND_HISTOGRAM, Opts). -spec create_observable_counter(Meter, Name, Opts) -> otel_instrument:t() when Meter :: t(), Name :: otel_instrument:name(), - Opts :: opts(). + Opts :: otel_instrument:opts(). create_observable_counter(Meter, Name, Opts) -> create_instrument(Meter, Name, ?KIND_OBSERVABLE_COUNTER, Opts). @@ -99,14 +95,14 @@ create_observable_counter(Meter, Name, Opts) -> Name :: otel_instrument:name(), Callback :: otel_instrument:callback(), CallbackArgs :: otel_instrument:callback_args(), - Opts :: opts(). + Opts :: otel_instrument:opts(). create_observable_counter(Meter, Name, Callback, CallbackArgs, Opts) -> create_instrument(Meter, Name, ?KIND_OBSERVABLE_COUNTER, Callback, CallbackArgs, Opts). -spec create_observable_gauge(Meter, Name, Opts) -> otel_instrument:t() when Meter :: t(), Name :: otel_instrument:name(), - Opts :: opts(). + Opts :: otel_instrument:opts(). create_observable_gauge(Meter, Name, Opts) -> create_instrument(Meter, Name, ?KIND_OBSERVABLE_GAUGE, Opts). @@ -115,14 +111,14 @@ create_observable_gauge(Meter, Name, Opts) -> Name :: otel_instrument:name(), Callback :: otel_instrument:callback(), CallbackArgs :: otel_instrument:callback_args(), - Opts :: opts(). + Opts :: otel_instrument:opts(). create_observable_gauge(Meter, Name, Callback, CallbackArgs, Opts) -> create_instrument(Meter, Name, ?KIND_OBSERVABLE_GAUGE, Callback, CallbackArgs, Opts). -spec create_observable_updowncounter(Meter, Name, Opts) -> otel_instrument:t() when Meter :: t(), Name :: otel_instrument:name(), - Opts :: opts(). + Opts :: otel_instrument:opts(). create_observable_updowncounter(Meter, Name, Opts) -> create_instrument(Meter, Name, ?KIND_OBSERVABLE_UPDOWNCOUNTER, Opts). @@ -131,7 +127,7 @@ create_observable_updowncounter(Meter, Name, Opts) -> Name :: otel_instrument:name(), Callback :: otel_instrument:callback(), CallbackArgs :: otel_instrument:callback_args(), - Opts :: opts(). + Opts :: otel_instrument:opts(). create_observable_updowncounter(Meter, Name, Callback, CallbackArgs, Opts) -> create_instrument(Meter, Name, ?KIND_OBSERVABLE_UPDOWNCOUNTER, Callback, CallbackArgs, Opts). @@ -145,7 +141,7 @@ scope(Meter={Module, _}) -> Meter :: t(), Name :: otel_instrument:name(), Kind :: otel_instrument:kind(), - Opts :: opts(). + Opts :: otel_instrument:opts(). create_instrument(Meter={Module, _}, Name, Kind, Opts) -> Module:create_instrument(Meter, Name, Kind, Opts). @@ -155,7 +151,7 @@ create_instrument(Meter={Module, _}, Name, Kind, Opts) -> Kind :: otel_instrument:kind(), Callback :: otel_instrument:callback(), CallbackArgs :: otel_instrument:callback_args(), - Opts :: opts(). + Opts :: otel_instrument:opts(). create_instrument(Meter={Module, _}, Name, Kind, Callback, CallbackArgs, Opts) -> Module:create_instrument(Meter, Name, Kind, Callback, CallbackArgs, Opts). diff --git a/apps/opentelemetry_api_experimental/src/otel_meter_noop.erl b/apps/opentelemetry_api_experimental/src/otel_meter_noop.erl index e9b01048..775822f9 100644 --- a/apps/opentelemetry_api_experimental/src/otel_meter_noop.erl +++ b/apps/opentelemetry_api_experimental/src/otel_meter_noop.erl @@ -41,9 +41,7 @@ register_callback(_Meter, _Instruments, _Callback, _CallbackArgs) -> ok. create_instrument(Meter, Name, Kind, Opts) -> - otel_instrument:new(?MODULE, Meter, Kind, Name, maps:get(description, Opts, undefined), - maps:get(unit, Opts, undefined)). + otel_instrument:new(?MODULE, Meter, Kind, Name, Opts). create_instrument(Meter, Name, Kind, Callback, CallbackArgs, Opts) -> - otel_instrument:new(?MODULE, Meter, Kind, Name, maps:get(description, Opts, undefined), - maps:get(unit, Opts, undefined), Callback, CallbackArgs). + otel_instrument:new(?MODULE, Meter, Kind, Name, Callback, CallbackArgs, Opts). diff --git a/apps/opentelemetry_api_experimental/src/otel_updown_counter.erl b/apps/opentelemetry_api_experimental/src/otel_updown_counter.erl index f143a929..64c73d0a 100644 --- a/apps/opentelemetry_api_experimental/src/otel_updown_counter.erl +++ b/apps/opentelemetry_api_experimental/src/otel_updown_counter.erl @@ -28,7 +28,7 @@ -spec create(Meter, Name, Opts) -> otel_instrument:t() when Meter :: otel_meter:t(), Name :: otel_instrument:name(), - Opts :: otel_meter:opts(). + Opts :: otel_instrument:opts(). create(Meter, Name, Opts) -> otel_meter:create_updown_counter(Meter, Name, Opts). diff --git a/apps/opentelemetry_experimental/src/otel_meter_default.erl b/apps/opentelemetry_experimental/src/otel_meter_default.erl index 05a67fc4..90f27363 100644 --- a/apps/opentelemetry_experimental/src/otel_meter_default.erl +++ b/apps/opentelemetry_experimental/src/otel_meter_default.erl @@ -34,12 +34,11 @@ -define(INSTRUMENT_NAME_REGEX, "^[A-Za-z]+[A-Za-z0-9/_.\-]{0,254}$"). --spec create_instrument(otel_meter:t(), otel_instrument:name(), otel_instrument:kind(), otel_meter:opts()) -> otel_instrument:t(). +-spec create_instrument(otel_meter:t(), otel_instrument:name(), otel_instrument:kind(), otel_instrument:opts()) -> otel_instrument:t(). create_instrument(Meter, Name, Kind, Opts) -> validate_name(Name), Instrument=#instrument{meter={_, #meter{provider=Provider}}} = - otel_instrument:new(?MODULE, Meter, Kind, Name, maps:get(description, Opts, undefined), - maps:get(unit, Opts, undefined)), + otel_instrument:new(?MODULE, Meter, Kind, Name, Opts), _ = otel_meter_server:add_instrument(Provider, Instrument), Instrument. @@ -52,12 +51,11 @@ lookup_instrument(Meter={_, #meter{instruments_tab=Tab}}, Name) -> undefined end. --spec create_instrument(otel_meter:t(), otel_instrument:name(), otel_instrument:kind(), otel_instrument:callback(), otel_instrument:callback_args(), otel_meter:opts()) -> otel_instrument:t(). +-spec create_instrument(otel_meter:t(), otel_instrument:name(), otel_instrument:kind(), otel_instrument:callback(), otel_instrument:callback_args(), otel_instrument:opts()) -> otel_instrument:t(). create_instrument(Meter, Name, Kind, Callback, CallbackArgs, Opts) -> validate_name(Name), Instrument=#instrument{meter={_, #meter{provider=Provider}}} = - otel_instrument:new(?MODULE, Meter, Kind, Name, maps:get(description, Opts, undefined), - maps:get(unit, Opts, undefined), Callback, CallbackArgs), + otel_instrument:new(?MODULE, Meter, Kind, Name, Callback, CallbackArgs, Opts), _ = otel_meter_server:add_instrument(Provider, Instrument), Instrument. From 3b406dce4459bdd17d34ac48625ddf47df913c14 Mon Sep 17 00:00:00 2001 From: Alberto Sartori Date: Tue, 19 Sep 2023 11:51:52 +0200 Subject: [PATCH 2/7] Validate advisory parameters --- .../src/otel_meter_default.erl | 33 +++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/apps/opentelemetry_experimental/src/otel_meter_default.erl b/apps/opentelemetry_experimental/src/otel_meter_default.erl index 90f27363..b23a2106 100644 --- a/apps/opentelemetry_experimental/src/otel_meter_default.erl +++ b/apps/opentelemetry_experimental/src/otel_meter_default.erl @@ -37,8 +37,9 @@ -spec create_instrument(otel_meter:t(), otel_instrument:name(), otel_instrument:kind(), otel_instrument:opts()) -> otel_instrument:t(). create_instrument(Meter, Name, Kind, Opts) -> validate_name(Name), + ValidatedOpts = validate_opts(Name, Kind, Opts), Instrument=#instrument{meter={_, #meter{provider=Provider}}} = - otel_instrument:new(?MODULE, Meter, Kind, Name, Opts), + otel_instrument:new(?MODULE, Meter, Kind, Name, ValidatedOpts), _ = otel_meter_server:add_instrument(Provider, Instrument), Instrument. @@ -54,8 +55,9 @@ lookup_instrument(Meter={_, #meter{instruments_tab=Tab}}, Name) -> -spec create_instrument(otel_meter:t(), otel_instrument:name(), otel_instrument:kind(), otel_instrument:callback(), otel_instrument:callback_args(), otel_instrument:opts()) -> otel_instrument:t(). create_instrument(Meter, Name, Kind, Callback, CallbackArgs, Opts) -> validate_name(Name), + ValidatedOpts = validate_opts(Name, Kind, Opts), Instrument=#instrument{meter={_, #meter{provider=Provider}}} = - otel_instrument:new(?MODULE, Meter, Kind, Name, Callback, CallbackArgs, Opts), + otel_instrument:new(?MODULE, Meter, Kind, Name, Callback, CallbackArgs, ValidatedOpts), _ = otel_meter_server:add_instrument(Provider, Instrument), Instrument. @@ -79,6 +81,33 @@ validate_name(Name) when is_atom(Name) -> validate_name(Name) -> ?LOG_ERROR("Invalid instrument name, should be an atom matching '~s', but got ~p", [?INSTRUMENT_NAME_REGEX, Name]), ok. + +validate_opts(Name, Kind, #{advisory_params := AdvisoryParams} = Opts) -> + ValidatedAdvisoryParams = maps:filtermap(fun(Key, Value) -> validate_advisory_param(Name, Kind, Key, Value) end, AdvisoryParams), + maps:put(advisory_params, ValidatedAdvisoryParams, Opts); +validate_opts(_Name, _Kind, Opts) -> + Opts. + +validate_advisory_param(Name, ?KIND_HISTOGRAM, explicit_bucket_boundaries, Value) -> + validate_explicit_bucket_boundaries(Name, Value); +validate_advisory_param(Name, _Kind, explicit_bucket_boundaries, _Value) -> + ?LOG_WARNING("[instrument '~s'] 'explicit_bucket_boundaries' advisory parameter is allowed only for histograms, ignoring", [Name]), + false; +validate_advisory_param(Name, _Kind, Opt, _Value) -> + ?LOG_WARNING("[instrument '~s'] '~s' advisory parameter is not supported, ignoring", [Name, Opt]), + false. + +validate_explicit_bucket_boundaries(Name, [_ | _] = Value) -> + case lists:all(fun is_number/1, Value) and (lists:sort(Value) == Value) of + true -> + {true, Value}; + false -> + ?LOG_WARNING("[instrument '~s'] 'explicit_bucket_boundaries' advisory parameter should be a not empty ordered list of numbers, got ~p", [Name, Value]), + false + end; +validate_explicit_bucket_boundaries(Name, Value) -> + ?LOG_WARNING("[instrument '~s'] 'explicit_bucket_boundaries' advisory parameter should be a not empty ordered list of numbers, got ~p", [Name, Value]), + false. %% record(Instrument=#instrument{meter={_, #meter{view_aggregations_tab=ViewAggregationTab, From d43a1f5de05efd5525076f2da1511e23efbcb067 Mon Sep 17 00:00:00 2001 From: Alberto Sartori Date: Tue, 19 Sep 2023 14:06:37 +0200 Subject: [PATCH 3/7] Use explicit_histogram_buckets if not configured in view --- .../src/otel_meter_default.erl | 5 +- .../src/otel_meter_server.erl | 2 - .../src/otel_view.erl | 16 +++- .../test/otel_metrics_SUITE.erl | 86 ++++++++++++++++++- 4 files changed, 101 insertions(+), 8 deletions(-) diff --git a/apps/opentelemetry_experimental/src/otel_meter_default.erl b/apps/opentelemetry_experimental/src/otel_meter_default.erl index b23a2106..656aaeda 100644 --- a/apps/opentelemetry_experimental/src/otel_meter_default.erl +++ b/apps/opentelemetry_experimental/src/otel_meter_default.erl @@ -83,7 +83,8 @@ validate_name(Name) -> ok. validate_opts(Name, Kind, #{advisory_params := AdvisoryParams} = Opts) -> - ValidatedAdvisoryParams = maps:filtermap(fun(Key, Value) -> validate_advisory_param(Name, Kind, Key, Value) end, AdvisoryParams), + % switch to maps:filtermap when we support only 24 onwards + ValidatedAdvisoryParams = maps:from_list(lists:filtermap(fun({Key, Value}) -> validate_advisory_param(Name, Kind, Key, Value) end, maps:to_list(AdvisoryParams))), maps:put(advisory_params, ValidatedAdvisoryParams, Opts); validate_opts(_Name, _Kind, Opts) -> Opts. @@ -100,7 +101,7 @@ validate_advisory_param(Name, _Kind, Opt, _Value) -> validate_explicit_bucket_boundaries(Name, [_ | _] = Value) -> case lists:all(fun is_number/1, Value) and (lists:sort(Value) == Value) of true -> - {true, Value}; + {true, {explicit_bucket_boundaries, Value}}; false -> ?LOG_WARNING("[instrument '~s'] 'explicit_bucket_boundaries' advisory parameter should be a not empty ordered list of numbers, got ~p", [Name, Value]), false diff --git a/apps/opentelemetry_experimental/src/otel_meter_server.erl b/apps/opentelemetry_experimental/src/otel_meter_server.erl index e99cd82b..d297e4f6 100644 --- a/apps/opentelemetry_experimental/src/otel_meter_server.erl +++ b/apps/opentelemetry_experimental/src/otel_meter_server.erl @@ -392,7 +392,6 @@ view_aggregation_for_reader(Instrument=#instrument{kind=Kind}, ViewAggregation, reader=Id, attribute_keys=AttributeKeys, aggregation_module=AggregationModule, - aggregation_options=#{}, temporality=Temporality}; view_aggregation_for_reader(Instrument=#instrument{kind=Kind}, ViewAggregation, View, Reader=#reader{id=Id, @@ -404,7 +403,6 @@ view_aggregation_for_reader(Instrument=#instrument{kind=Kind}, ViewAggregation, reader=Id, attribute_keys=undefined, aggregation_module=AggregationModule, - aggregation_options=#{}, temporality=Temporality}. diff --git a/apps/opentelemetry_experimental/src/otel_view.erl b/apps/opentelemetry_experimental/src/otel_view.erl index c2e9e960..cc4749f6 100644 --- a/apps/opentelemetry_experimental/src/otel_view.erl +++ b/apps/opentelemetry_experimental/src/otel_view.erl @@ -78,7 +78,8 @@ new(Name, Criteria, Config) -> -spec match_instrument_to_views(otel_instrument:t(), [t()]) -> [{t() | undefined, #view_aggregation{}}]. match_instrument_to_views(Instrument=#instrument{name=InstrumentName, meter=Meter, - description=Description}, Views) -> + description=Description, + advisory_params=AdvisoryParams}, Views) -> IsMonotonic = otel_instrument:is_monotonic(Instrument), Temporality = otel_instrument:temporality(Instrument), Scope = otel_meter:scope(Meter), @@ -91,6 +92,7 @@ match_instrument_to_views(Instrument=#instrument{name=InstrumentName, [] -> false; _ -> + AggregationOptions1 = aggragation_options(AggregationOptions, AdvisoryParams), %% `reader' needs to be undefined and is set %% for each in `otel_meter_server' %% eqwalizer:ignore see above @@ -101,20 +103,21 @@ match_instrument_to_views(Instrument=#instrument{name=InstrumentName, temporality=Temporality, is_monotonic=IsMonotonic, attribute_keys=AttributeKeys, - aggregation_options=AggregationOptions, + aggregation_options=AggregationOptions1, description=value_or(ViewDescription, Description) }}} end end, Views) of [] -> + AggregationOptions1 = aggragation_options(#{}, AdvisoryParams), [{undefined, #view_aggregation{name=InstrumentName, scope=Scope, instrument=Instrument, temporality=Temporality, is_monotonic=IsMonotonic, attribute_keys=undefined, - aggregation_options=#{}, + aggregation_options=AggregationOptions1, description=Description}}]; Aggs -> Aggs @@ -122,6 +125,13 @@ match_instrument_to_views(Instrument=#instrument{name=InstrumentName, %% +aggragation_options(#{boundaries := _} = AggregationOptions, _AdvisoryParams) -> + AggregationOptions; +aggragation_options(AggregationOptions, #{explicit_bucket_boundaries := Boundaries}) -> + maps:put(boundaries, Boundaries, AggregationOptions); +aggragation_options(AggregationOptions, _AdvisoryParams) -> + AggregationOptions. + value_or(undefined, Other) -> Other; value_or(Value, _Other) -> diff --git a/apps/opentelemetry_experimental/test/otel_metrics_SUITE.erl b/apps/opentelemetry_experimental/test/otel_metrics_SUITE.erl index 73638f13..e4ae264f 100644 --- a/apps/opentelemetry_experimental/test/otel_metrics_SUITE.erl +++ b/apps/opentelemetry_experimental/test/otel_metrics_SUITE.erl @@ -83,7 +83,7 @@ all() -> kill_reader, kill_server, observable_counter, observable_updown_counter, observable_gauge, multi_instrument_callback, using_macros, float_counter, float_updown_counter, float_histogram, sync_filtered_attributes, async_filtered_attributes, delta_observable_counter, - bad_observable_return, default_resource + bad_observable_return, default_resource, advisory_params ]. init_per_suite(Config) -> @@ -1130,3 +1130,87 @@ bad_observable_return(_Config) -> ?assertSumReceive(CounterName2, <<"observable counter 2 description">>, kb, [{8, #{}}]), ok. + +advisory_params(_Config) -> + DefaultMeter = otel_meter_default, + + Meter = opentelemetry_experimental:get_meter(), + ?assertMatch({DefaultMeter, _}, Meter), + + % explicit_bucket_boundaries allowed only for histograms + Counter = otel_counter:create(Meter, invalid_1, + #{advisory_params => #{explicit_bucket_boundaries => [1, 2, 3]}}), + ?assertEqual(Counter#instrument.advisory_params, #{}), + + % advisory parameters different from explicit_bucket_boundaries are not allowed + Counter1 = otel_counter:create(Meter, invalid_2, #{advisory_params => #{invalid => invalid}}), + ?assertEqual(Counter1#instrument.advisory_params, #{}), + + % explicit_bucket_boundaries should be an ordered list of numbers + Histo1 = otel_histogram:create(Meter, invalid_3, + #{advisory_params => #{explicit_bucket_boundaries => invalid}}), + ?assertEqual(Histo1#instrument.advisory_params, #{}), + + Histo2 = otel_histogram:create(Meter, invalid_4, + #{advisory_params => #{explicit_bucket_boundaries => [2,1,4]}}), + ?assertEqual(Histo2#instrument.advisory_params, #{}), + + % when valid use the explicit_bucket_boundaries from advisory_params if not set in a view + Histogram = otel_histogram:create(Meter, a_histogram, + #{advisory_params => #{explicit_bucket_boundaries => [10, 20, 30]}}), + ?assertEqual(Histogram#instrument.advisory_params, #{explicit_bucket_boundaries => [10, 20, 30]}), + + ?assertEqual(ok, otel_histogram:record(Histogram, 15, #{<<"a">> => <<"1">>})), + ?assertEqual(ok, otel_histogram:record(Histogram, 50, #{<<"a">> => <<"1">>})), + ?assertEqual(ok, otel_histogram:record(Histogram, 26, #{<<"a">> => <<"2">>})), + + otel_meter_server:force_flush(), + + receive + {otel_metric, #metric{name=a_histogram, + data=#histogram{datapoints=Datapoints}}} -> + AttributeBuckets = + lists:sort([{Attributes, Buckets, Min, Max, Sum} || #histogram_datapoint{bucket_counts=Buckets, + attributes=Attributes, + min=Min, + max=Max, + sum=Sum} <- Datapoints]), + ?assertEqual([], [{#{<<"a">> => <<"1">>}, [0,1,0,1], 15, 50, 65}, + {#{<<"a">> => <<"2">>}, [0,0,1,0], 26, 26, 26}] + -- AttributeBuckets, AttributeBuckets) + after + 5000 -> + ct:fail(histogram_receive_timeout) + end, + + % boundaries from view have precedence + ?assert(otel_meter_server:add_view(view, #{instrument_name => b_histogram}, #{ + aggregation_module => otel_aggregation_histogram_explicit, + aggregation_options => #{boundaries => [10, 100]}})), + + HistogramB = otel_histogram:create(Meter, b_histogram, + #{advisory_params => #{explicit_bucket_boundaries => [10, 20, 30]}}), + ?assertEqual(HistogramB#instrument.advisory_params, #{explicit_bucket_boundaries => [10, 20, 30]}), + + ?assertEqual(ok, otel_histogram:record(HistogramB, 15, #{<<"a">> => <<"1">>})), + ?assertEqual(ok, otel_histogram:record(HistogramB, 50, #{<<"a">> => <<"1">>})), + ?assertEqual(ok, otel_histogram:record(HistogramB, 26, #{<<"a">> => <<"2">>})), + + otel_meter_server:force_flush(), + + receive + {otel_metric, #metric{name=view, + data=#histogram{datapoints=DatapointsB}}} -> + AttributeBucketsB = + lists:sort([{Attributes, Buckets, Min, Max, Sum} || #histogram_datapoint{bucket_counts=Buckets, + attributes=Attributes, + min=Min, + max=Max, + sum=Sum} <- DatapointsB]), + ?assertEqual([], [{#{<<"a">> => <<"1">>}, [0,2,0], 15, 50, 65}, + {#{<<"a">> => <<"2">>}, [0,1,0], 26, 26, 26}] + -- AttributeBucketsB, AttributeBucketsB) + after + 1000 -> + ct:fail(histogram_receive_timeout) + end. \ No newline at end of file From 34e827175723e693dfdec2788d367fe0ba070baa Mon Sep 17 00:00:00 2001 From: Alberto Sartori Date: Sat, 23 Sep 2023 15:41:55 +0200 Subject: [PATCH 4/7] Add test about histogram aggregation option --- .../test/otel_metrics_SUITE.erl | 37 ++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/apps/opentelemetry_experimental/test/otel_metrics_SUITE.erl b/apps/opentelemetry_experimental/test/otel_metrics_SUITE.erl index e4ae264f..c9dc1c18 100644 --- a/apps/opentelemetry_experimental/test/otel_metrics_SUITE.erl +++ b/apps/opentelemetry_experimental/test/otel_metrics_SUITE.erl @@ -83,7 +83,7 @@ all() -> kill_reader, kill_server, observable_counter, observable_updown_counter, observable_gauge, multi_instrument_callback, using_macros, float_counter, float_updown_counter, float_histogram, sync_filtered_attributes, async_filtered_attributes, delta_observable_counter, - bad_observable_return, default_resource, advisory_params + bad_observable_return, default_resource, histogram_aggregation_options, advisory_params ]. init_per_suite(Config) -> @@ -1198,6 +1198,41 @@ advisory_params(_Config) -> otel_meter_server:force_flush(), + receive + {otel_metric, #metric{name=view, + data=#histogram{datapoints=DatapointsB}}} -> + AttributeBucketsB = + lists:sort([{Attributes, Buckets, Min, Max, Sum} || #histogram_datapoint{bucket_counts=Buckets, + attributes=Attributes, + min=Min, + max=Max, + sum=Sum} <- DatapointsB]), + ?assertEqual([], [{#{<<"a">> => <<"1">>}, [0,2,0], 15, 50, 65}, + {#{<<"a">> => <<"2">>}, [0,1,0], 26, 26, 26}] + -- AttributeBucketsB, AttributeBucketsB) + after + 1000 -> + ct:fail(histogram_receive_timeout) + end. + +histogram_aggregation_options(_Config) -> + DefaultMeter = otel_meter_default, + + Meter = opentelemetry_experimental:get_meter(), + ?assertMatch({DefaultMeter, _}, Meter), + + ?assert(otel_meter_server:add_view(view, #{instrument_name => histogram}, #{ + aggregation_module => otel_aggregation_histogram_explicit, + aggregation_options => #{boundaries => [10, 100]}})), + + Histogram = otel_histogram:create(Meter, histogram, #{}), + + ?assertEqual(ok, otel_histogram:record(Histogram, 15, #{<<"a">> => <<"1">>})), + ?assertEqual(ok, otel_histogram:record(Histogram, 50, #{<<"a">> => <<"1">>})), + ?assertEqual(ok, otel_histogram:record(Histogram, 26, #{<<"a">> => <<"2">>})), + + otel_meter_server:force_flush(), + receive {otel_metric, #metric{name=view, data=#histogram{datapoints=DatapointsB}}} -> From 046d97c13747bb59893869e9f4656c9c897da0e4 Mon Sep 17 00:00:00 2001 From: Alberto Sartori Date: Sat, 23 Sep 2023 16:47:03 +0200 Subject: [PATCH 5/7] fix --- apps/opentelemetry_api_experimental/src/otel_instrument.erl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/apps/opentelemetry_api_experimental/src/otel_instrument.erl b/apps/opentelemetry_api_experimental/src/otel_instrument.erl index c728ffe1..6feac48a 100644 --- a/apps/opentelemetry_api_experimental/src/otel_instrument.erl +++ b/apps/opentelemetry_api_experimental/src/otel_instrument.erl @@ -40,9 +40,9 @@ -type advisory_params() :: #{explicit_bucket_boundaries => [number(), ...]}. --type opts() :: #{description => otel_instrument:description(), - unit => otel_instrument:unit(), - advisory_params => otel_instrument:advisory_params()}. +-type opts() :: #{description => description(), + unit => unit(), + advisory_params => advisory_params()}. -type t() :: #instrument{}. From 5e48148cbc8185e95300e7f8619e0a975be300b2 Mon Sep 17 00:00:00 2001 From: Alberto Sartori Date: Mon, 2 Oct 2023 09:22:43 +0200 Subject: [PATCH 6/7] Rename boundaries to explicit_bucket_boundaries --- .../include/otel_metrics.hrl | 2 +- .../otel_aggregation_histogram_explicit.erl | 20 +++++++++---------- .../src/otel_view.erl | 4 ++-- .../test/otel_metrics_SUITE.erl | 6 +++--- 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/apps/opentelemetry_experimental/include/otel_metrics.hrl b/apps/opentelemetry_experimental/include/otel_metrics.hrl index ba48230c..16c008f2 100644 --- a/apps/opentelemetry_experimental/include/otel_metrics.hrl +++ b/apps/opentelemetry_experimental/include/otel_metrics.hrl @@ -62,7 +62,7 @@ start_time_unix_nano :: integer() | {const, eqwalizer:dynamic()} | '$9' | '$2' | 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] - boundaries :: [float()] | match_spec([float()]), + 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{}}, bucket_counts :: counters:counters_ref() | match_spec(undefined), diff --git a/apps/opentelemetry_experimental/src/otel_aggregation_histogram_explicit.erl b/apps/opentelemetry_experimental/src/otel_aggregation_histogram_explicit.erl index 7981e556..cd8ade8c 100644 --- a/apps/opentelemetry_experimental/src/otel_aggregation_histogram_explicit.erl +++ b/apps/opentelemetry_experimental/src/otel_aggregation_histogram_explicit.erl @@ -136,12 +136,12 @@ init(#view_aggregation{name=Name, reader=ReaderId, aggregation_options=Options}, Attributes) -> Key = {Name, Attributes, ReaderId}, - Boundaries = maps:get(boundaries, Options, ?DEFAULT_BOUNDARIES), + ExplicitBucketBoundaries = maps:get(explicit_bucket_boundaries, Options, ?DEFAULT_BOUNDARIES), RecordMinMax = maps:get(record_min_max, Options, true), #explicit_histogram_aggregation{key=Key, start_time_unix_nano=erlang:system_time(nanosecond), - boundaries=Boundaries, - bucket_counts=new_bucket_counts(Boundaries), + explicit_bucket_boundaries=ExplicitBucketBoundaries, + bucket_counts=new_bucket_counts(ExplicitBucketBoundaries), checkpoint=undefined, record_min_max=RecordMinMax, min=infinity, %% works because any atom is > any integer @@ -153,17 +153,17 @@ aggregate(Table, #view_aggregation{name=Name, reader=ReaderId, aggregation_options=Options}, Value, Attributes) -> Key = {Name, Attributes, ReaderId}, - Boundaries = maps:get(boundaries, Options, ?DEFAULT_BOUNDARIES), + ExplicitBucketBoundaries = maps:get(explicit_bucket_boundaries, Options, ?DEFAULT_BOUNDARIES), try ets:lookup_element(Table, Key, #explicit_histogram_aggregation.bucket_counts) of BucketCounts0 -> BucketCounts = case BucketCounts0 of undefined -> - new_bucket_counts(Boundaries); + new_bucket_counts(ExplicitBucketBoundaries); _ -> BucketCounts0 end, - BucketIdx = find_bucket(Boundaries, Value), + BucketIdx = find_bucket(ExplicitBucketBoundaries, Value), counters:add(BucketCounts, BucketIdx, 1), MS = ?AGGREATE_MATCH_SPEC(Key, Value, BucketCounts), @@ -181,7 +181,7 @@ checkpoint(Tab, #view_aggregation{name=Name, temporality=?TEMPORALITY_DELTA}, CollectionStartNano) -> MS = [{#explicit_histogram_aggregation{key='$1', start_time_unix_nano='$9', - boundaries='$2', + explicit_bucket_boundaries='$2', record_min_max='$3', checkpoint='_', bucket_counts='$5', @@ -193,7 +193,7 @@ checkpoint(Tab, #view_aggregation{name=Name, {'=:=', {element, 3, '$1'}, {const, ReaderId}}], [{#explicit_histogram_aggregation{key='$1', start_time_unix_nano={const, CollectionStartNano}, - boundaries='$2', + explicit_bucket_boundaries='$2', record_min_max='$3', checkpoint={#explicit_histogram_checkpoint{bucket_counts='$5', min='$6', @@ -229,7 +229,7 @@ collect(Tab, #view_aggregation{name=Name, datapoint(CollectionStartNano, #explicit_histogram_aggregation{ key={_, Attributes, _}, - boundaries=Boundaries, + explicit_bucket_boundaries=Boundaries, start_time_unix_nano=StartTimeUnixNano, checkpoint=undefined, bucket_counts=BucketCounts, @@ -253,7 +253,7 @@ datapoint(CollectionStartNano, #explicit_histogram_aggregation{ }; datapoint(CollectionStartNano, #explicit_histogram_aggregation{ key={_, Attributes, _}, - boundaries=Boundaries, + explicit_bucket_boundaries=Boundaries, checkpoint=#explicit_histogram_checkpoint{bucket_counts=BucketCounts, min=Min, max=Max, diff --git a/apps/opentelemetry_experimental/src/otel_view.erl b/apps/opentelemetry_experimental/src/otel_view.erl index cc4749f6..9eaa24ac 100644 --- a/apps/opentelemetry_experimental/src/otel_view.erl +++ b/apps/opentelemetry_experimental/src/otel_view.erl @@ -125,10 +125,10 @@ match_instrument_to_views(Instrument=#instrument{name=InstrumentName, %% -aggragation_options(#{boundaries := _} = AggregationOptions, _AdvisoryParams) -> +aggragation_options(#{explicit_bucket_boundaries := _} = AggregationOptions, _AdvisoryParams) -> AggregationOptions; aggragation_options(AggregationOptions, #{explicit_bucket_boundaries := Boundaries}) -> - maps:put(boundaries, Boundaries, AggregationOptions); + maps:put(explicit_bucket_boundaries, Boundaries, AggregationOptions); aggragation_options(AggregationOptions, _AdvisoryParams) -> AggregationOptions. diff --git a/apps/opentelemetry_experimental/test/otel_metrics_SUITE.erl b/apps/opentelemetry_experimental/test/otel_metrics_SUITE.erl index c9dc1c18..6308472f 100644 --- a/apps/opentelemetry_experimental/test/otel_metrics_SUITE.erl +++ b/apps/opentelemetry_experimental/test/otel_metrics_SUITE.erl @@ -1183,10 +1183,10 @@ advisory_params(_Config) -> ct:fail(histogram_receive_timeout) end, - % boundaries from view have precedence + % explicit_bucket_boundaries from view have precedence ?assert(otel_meter_server:add_view(view, #{instrument_name => b_histogram}, #{ aggregation_module => otel_aggregation_histogram_explicit, - aggregation_options => #{boundaries => [10, 100]}})), + aggregation_options => #{explicit_bucket_boundaries => [10, 100]}})), HistogramB = otel_histogram:create(Meter, b_histogram, #{advisory_params => #{explicit_bucket_boundaries => [10, 20, 30]}}), @@ -1223,7 +1223,7 @@ histogram_aggregation_options(_Config) -> ?assert(otel_meter_server:add_view(view, #{instrument_name => histogram}, #{ aggregation_module => otel_aggregation_histogram_explicit, - aggregation_options => #{boundaries => [10, 100]}})), + aggregation_options => #{explicit_bucket_boundaries => [10, 100]}})), Histogram = otel_histogram:create(Meter, histogram, #{}), From 8c09768bc147cd1262213cf98d25a90d24ebdca7 Mon Sep 17 00:00:00 2001 From: Alberto Sartori Date: Mon, 2 Oct 2023 09:26:00 +0200 Subject: [PATCH 7/7] Add Changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d0cd8b70..4bf793f4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changes - [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 @@ -37,6 +38,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - [Add `instrument_unit` to view criteria](https://github.com/open-telemetry/opentelemetry-erlang/pull/604) - [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) ### Changes