Use explicit_histogram_buckets if not configured in view

open-telemetry · Sep 19, 2023 · d43a1f5 · d43a1f5
1 parent 3b406dc
commit d43a1f5
Show file tree

Hide file tree

Showing 4 changed files with 101 additions and 8 deletions.
diff --git 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
@@ -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
@@ -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,27 +103,35 @@ 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
     end.
 
 %%
 
+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
@@ -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.