From d43a1f5de05efd5525076f2da1511e23efbcb067 Mon Sep 17 00:00:00 2001 From: Alberto Sartori Date: Tue, 19 Sep 2023 14:06:37 +0200 Subject: [PATCH] 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