API design question for metrics instrument usage

[tsloughter]

Currently the macros like counter_add do:

-define(counter_add(NameOrInstrument, Number, Attributes),
        case is_atom(NameOrInstrument) of 
            true -> 
                otel_counter:add(otel_ctx:get_current(), ?current_meter, NameOrInstrument, Number, Attributes); 
            false -> 
                otel_counter:add(otel_ctx:get_current(), NameOrInstrument, Number, Attributes) 
        end).

This, rightly, upsets dialyzer as we can see in the contrib example roll_dice_elli:

?counter_add(?ROLL_COUNTER, 1, #{'roll.value' => Roll}),

Line 40 Column 38: The pattern 'false' can never match the type 'true'

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 to otel_counter:add so it can do the ?current_meter itself...

otel_counter:add(otel_ctx:get_current(), ?MODULE, NameOrInstrument, Number, Attributes); 

[ferd]

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:

opentelemetry-erlang/apps/opentelemetry_api_experimental/src/otel_counter.erl

Lines 40 to 48 in e6c5c75

	-spec add(
	otel_ctx:t(), otel_meter:t() | otel_instrument:t(),
	otel_instrument:name() | pos_integer() | float(),
	pos_integer() | float() | opentelemetry:attributes_map()) -> ok.
	add(Ctx, Instrument=#instrument{module=Module}, Number, Attributes) ->
	Module:record(Ctx, Instrument, Number, Attributes);
	add(Ctx, Meter, Name, Number) ->
	otel_meter:record(Ctx, Meter, Name, Number).

I don't think adding a similar match in add/5 would be a problem, if anything it may increase consistency?

[tsloughter]

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.

[ferd]

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?

[tsloughter]

Hm, interesting idea.

Yea, arity confusion. Have messed that up a few places. Maybe a reason to make it separate calls. ?instrument_counter_add(...).

[ferd]

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.