Improve spec compliance

[albertored]

1. Attributes are optional in Counter.add(), UpDownCounter.add() and Histo.record()
2. Counter.add() and Histogram.record() accept only positive numbers

For the second point I only log and discard the value, should we also return an error? If so it will happen only when the SDK is present so I think we are spec compliant. The following is the only part of spec I found mentioning this

The increment value is expected to be non-negative. This API SHOULD be documented in a way to communicate to users that this value is expected to be non-negative. This API SHOULD NOT validate this value, that is left to implementations of the API.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (311948a) 73.02% compared to head (b94ff50) 72.94%.

❗ Current head b94ff50 differs from pull request most recent head 750e47a. Consider uploading reports for the commit 750e47a to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #632      +/-   ##
==========================================
- Coverage   73.02%   72.94%   -0.09%     
==========================================
  Files          61       61              
  Lines        1924     1918       -6     
==========================================
- Hits         1405     1399       -6     
  Misses        519      519              

Flag	Coverage Δ
api	69.64% <ø> (ø)
elixir	17.47% <ø> (ø)
erlang	74.26% <ø> (-0.09%)	⬇️
exporter	66.66% <ø> (-0.82%)	⬇️
sdk	78.69% <ø> (ø)
zipkin	54.16% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tsloughter]

The checking of positive numbers is actually a mistake in the matrix.

I see you put it in the SDK at least but the matrix I think is referring to the API.

We already drop negative values in aggregators if the aggregation is monotonic.

[albertored]

I think the matrix is referring to this part of the spec:
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#L541

I'm not sure this is the same as dropping negative values on monotonic aggregations

[tsloughter]

Right, that part of the spec says:

This API SHOULD NOT validate this value

[albertored]

Yes, my interpretation of

that is left to implementations of the API

was that the validation should be done on SDK.

Not clear to me why this validation is done on aggregations. Monotonic is a property of the instrument not of the aggregation, in addition at the moment the check for positive values is done only on sum aggregation (checked from smartphone so I may have missed something)

[tsloughter]

Ah yea, its only done on the sum aggregation right now.

[albertored]

@tsloughter changelog updated. I also removed the check for positive values on the sum aggregation module since it is now already done elsewhere. If you prefer I can revert that last commit but to me it seems more clear in this way

[tsloughter]

Well that is concerning... tests pass on 24 but not 26 and the failure is unexpected metric results.

[tsloughter]

%%% otel_metrics_SUITE ==> float_updown_counter: FAILED
%%% otel_metrics_SUITE ==> 
Failure/Error: ?assertMatch([ ], lists : sort ( [ { 3.3 , # { } } , { 10.0 , # { << "c" >> => << "b" >> } } ] ) -- SortedDatapoints)
  expected: = [ ]
       got: [{3.3,#{}},{10.0,#{<<"c">> => <<"b">>}}]
      line: 305
   comment: [{5.4,#{}},{15.4,#{<<"c">> => <<"b">>}}]

So the test expects 3.3 and 10.0 but gets 5.4 and 15.4. I don't even see how those numbers are possible from the recordings made.

[tsloughter]

Wonder if this shouldn't be a debug log instead? To guard against a messed up dependency flooding info logs with logs not about actual functionality shrug

[albertored]

Yea I'm always dubious about the level. I got your point but on the other side a debug log is hardly seen by the user. So I'm ok both with debug and info, we should also align this all over the code

[albertored]

Really strange indeed, I'll take a look

[albertored]

@tsloughter tests are running in while loop since a while with the same version of CI (26.1.2) and they are consistently passing. Can you try to re-trigger the CI and run them locally?

[tsloughter]

Yea, they pass, makes me fear a race condition.

[tsloughter]

Merged but opened #661