OpenTelemetry Gradualization

[tsloughter]

This is about issues I'm hitting as I attempt to take care of all the errors from gradualizer in https://github.com/open-telemetry/opentelemetry-erlang

I'm opening this discussion to not flood the either issues or discussions with what often turn up to be my mistakes (like the last issue I opened and closed :). I usually catch myself before opening an issue but figure better to just open a discussion to centralize and I'll open issues if anything is actually found to be an issue.

The first one I'll start with is more a question of what can I do to silence it :). I get this error for concat:

otel_metric_reader.beam: The operator '++' on line 175 at column 27 is expected to have type _TyVar-576460752303409663 which is too precise to be statically checked
otel_metric_reader.beam: The operator '++' on line 188 at column 87 is expected to have type _TyVar-576460752303409215 which is too precise to be statically checked

Code in question: https://github.com/open-telemetry/opentelemetry-erlang/blob/main/apps/opentelemetry_experimental/src/otel_metric_reader.erl

When I'd get too precise errors elsewhere I could just use ?assert_type(...), but here the type is a variable (not sure why the type it is looking for isn't a list?)

[tsloughter]

I'm also not sure what to do about this one :)

otel_baggage.beam: The function call on line 56 at column 5 is expected to have type otel_ctx:t() | ok but it has type otel_ctx:t() | ok

    when (is_list(Key) orelse is_binary(Key)) andalso
             is_binary(Value) ->
    set(Key, Value, []);
    ^^^^^^^^^^^^^^^^^^^

[tsloughter]

Interesting, hit the same thing in another part of code:

otel_view.beam: The record field on line 56 at column 25 is expected to have type otel_instrument:name() | '_' | undefined but it has type otel_instrument:name() | '_' | undefined

new(Criteria, Config) ->
    Selection = criteria_to_selection(Criteria),
    #view{name = Selection#selection.instrument_name,

[tsloughter]

I was able to silence these by asserting the type is the expanded version of the type, so like instead of otel_ctx:t() I assert it is undefined | #{any() => any()}:

?assert_type(set(Key, Value, []), ok | undefined | #{any() => any()});

and for the other:

#view{name=?assert_type(case Selection#selection.instrument_name of '_' -> undefined; N -> N end, undefined | unicode:latin1_binary() | unicode:latin1_charlist())

Guessing something is up with knowing uses of a module exported type is the same or something like that?

[tsloughter]

This one might be an actual case of wanting to allow any user input but gradualizer, rightly, complaining that a clause can't be reached:

otel_propagator_b3.beam: The clause on line 90 at column 1 cannot be reached

-spec inject(Context, Carrier, CarrierSetFun, Options) -> Carrier
              when Context :: otel_ctx:t(),
                   Carrier :: otel_propagator:carrier(),
                   CarrierSetFun :: otel_propagator_text_map:carrier_set(),
                   Options :: b3multi | b3single.
inject(Ctx, Carrier, CarrierSet, Options=b3single) ->
    otel_propagator_b3single:inject(Ctx, Carrier, CarrierSet, Options);
inject(Ctx, Carrier, CarrierSet, Options=b3multi) ->
    otel_propagator_b3multi:inject(Ctx, Carrier, CarrierSet, Options);
inject(_Ctx, Carrier, _CarrierSet, _Options) ->
    Carrier.

I'm guessing for these what is needed is per-function ignore options like dialyzer, https://www.erlang.org/doc/man/dialyzer.html#suppression

I might be able to do something similar to what was done before dialyzer supported this natively by putting them in the rebar.config and filtering errors after the fact before printing them out.

[tsloughter]

Yes, like that.

That work around should help me in some places, but it'll mean not catching errors in any place that the record is directly defined, right?

[zuiderkwast]

it'll mean not catching errors in any place that the record is directly defined, right?

Yes, I think that's the case unfortunately, unless we can come up with a better workaround.

I'm fine with adding an experimental option for ignoring this kind of errors in match specs, but I don't think it should be enabled by default. You want to silence them in records appearing syntactically in args to calls to etc:select, etc:match etc.? Meaning if the match spec is in a variable (like M = #r{f = '$1'}, etc:match(tab, M)), it won't be silenced?

[tsloughter]

Right, about variable usage, definitely not perfect. But I don't know if that is reason to make it not the default, since the user would be able to be told they can move it into the select/match to silence it, or do one of the work arounds. But in the cases where it is definitely a matchspec usage will be silent.

Unless the variable can be typed as a match_pattern() since it gets passed later to match? Sorry, don't know enough (or much of anything!) about gradual typing and gradualizer (yet!).

[zuiderkwast]

To take it further, a match spec record could be passed around between functions, stored in various ways and there will be situations where won't be able to infer that it's a match spec.

But that's not the reason I think such option shouldn't be enabled by default. The reason is that I think records as match specs are a hack. Match specs not part of the core language. It's just a convention and it takes advantage of the dynamic type system of Erlang. In a statically typed language, it wouldn't work to replace a typed record field value or tuple element with a wildcard value like '_' or '$1' which is actually a value of a different type. If we add this feature, it would be practical, but theoretically wrong...

But I'm open to have it on by default if others think it's a good idea.

[josefs]

It would be an interesting project to design an API for typed match specs in Erlang. If Gradualizer catches on enough then people will want such an API eventually.

[zuiderkwast]

I'm fine with adding the same for gradualizer: -gradualizer({nowarn_function, select_expired/1}).

[tsloughter]

This may be the last one. Code:

-spec merge(t(), t()) -> t().
merge(#resource{schema_url=NewSchemaUrl,
                attributes=NewAttributes}, Current=#resource{schema_url=CurrentSchemaUrl,
                                                             attributes=CurrentAttributes}) ->
    SchameUrl = merge_schema_url(NewSchemaUrl, CurrentSchemaUrl),
    NewMap = otel_attributes:map(NewAttributes),
    Current#resource{schema_url=SchameUrl,
                     attributes=otel_attributes:set(NewMap, CurrentAttributes)}.

Error:

otel_resource.beam: Nonexhaustive patterns on line 77 at column 1
Example values which are not covered:
        #resource{}
        #resource{}

[tsloughter]

The error is for the function definition of merge/2.