Open tsloughter opened 1 month ago
right, because the case expression is fully local, the branch is unique to each call-site.
I'm noticing that we already do some dynamic dispatch on otel_counter:add/4
: https://github.com/open-telemetry/opentelemetry-erlang/blob/e6c5c756d9a5df3714c5f379fcdae3ab1fae7980/apps/opentelemetry_api_experimental/src/otel_counter.erl#L40-L48
I don't think adding a similar match in add/5
would be a problem, if anything it may increase consistency?
The problem is the use of ?MODULE. We'd have to always pass the module for the meter to maybe be looked up in the counter module.
An alternative is to make it a sort of "private API" function that the macro uses to create a layer of indirection but passes the union of all required context to.
like
-define(counter_add(NameOrInstrument, Number, Attributes),
otel_counter:'_macro_helper_add'(otel_ctx:get_current(), ?current_meter, NameOrInstrument, Number, Attributes)).
% [...] within otel_counter, maybe?
'_macro_helper_add'(Ctx, Meter, NameOrInstrument, Number, Attributes) ->
case is_atom(NameOrInstrument) of
true ->
add(Ctx, Meter, NameOrInstrument, Number, Attributes);
false ->
add(Ctx, NameOrInstrument, Number, Attributes)
end).
This looks like hot garbage internally but keeps macro usage unchanged.
I would however say that the arity confusion likely transfers to users of the app who aren't quite sure when to pass in the right data types?
Hm, interesting idea.
Yea, arity confusion. Have messed that up a few places. Maybe a reason to make it separate calls. ?instrument_counter_add(...)
.
Yeah separate calls would have a clear demarcation of type signatures and split docs automatically. Sounds like it's a bit wordier but clearer for usage.
Currently the macros like
counter_add
do:This, rightly, upsets dialyzer as we can see in the contrib example
roll_dice_elli
:I think we need to split the macro into 2 for each instrument but I'm not sure what to name them so opening this issue :).
I suppose another option is to always pass
?MODULE
tootel_counter:add
so it can do the?current_meter
itself...