Spatial aggregation for async instruments with filtering views

[fandreuz]

Fixes #4901

I propose the following changes:

• AsynchronousMetricStorage now declares void record(Attributes, long) and void record(Attributes, double) instead of a single void record(Measurement), to enable using AggregatorHandle within the class.
• AsynchronousMetricStorage now contains two long fields (startEpochNanos, epochNanos) which are injected before running the callbacks, as an alternative to passing a Measurement object.
• Some tests had to change due to the change in the interface of AsynchronousMetricStorage

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: fandreuz / name: Francesco Andreuzzi (944b3ef, ed783f9, 6816f91, 8bde0f4, ab8bd4d, 4254d30, 9dcaf3b, fa58b02, 0dca607, f831572, 561e479, 4fe5834, 0c63775, e942dc2, cbe0784, 63f2471, 4e566d7, 1609f5f, 1f9a4b5, 29f93c5, 4613e7c, 7115187, 6e96eda, a5d8b67, 1bc1d78, 1371683, 4b3f874, 53ad5dd, f6f1db7)
• ✅ login: jack-berg (44b9825)

[fandreuz]

I'd like to discuss the test here. I would expect a cumulative sum instrument to aggregate all occurrences with empty attributes. Thus, I would expect to see 6 here instead of 3.

To preserve this behavior, I clear the values for all the attributes at every collection, which is why AsynchronousMetricStorageTest#collect_reusableData_reusedObjectsAreReturnedOnSecondCall is failing. I'd like to clarify this aspect before fixing it.

[jack-berg]

I'd like to discuss the test here. I would expect a cumulative sum instrument to aggregate all occurrences with empty attributes. Thus, I would expect to see 6 here instead of 3.

Yes I believe you're correct. The test / logic seem appear to be incorrect currently

Oh I forgot about a fundamental detail of asynchronous instruments: conceptually, when you make an observation with a cumulative instrument, we say that you are observing a cumulative value. Thus, if we have a reader with cumulative temporality which reads twice and invokes the callbacks twice, and the callback records the same value 3 each time, the reported value should indeed be 3 and not 6.

Consider it from the perspective of this instrumentation which records information about classes loaded. If in sequential reads the callback records that 100 classes have been loaded, the output of a cumulative reader should always be 100, not N*100 (where N is the number of times the callback has been invoked). Instrumentation is not responsible for managing state and recording only the delta.

[jack-berg]

Thanks for working on this. This is pretty complex code when you consider the combinations of cumulative + delta, reusable + immutable data mode, and our desire to maintain zero memory allocations with reusable data mode after the application reaches steady state.

So reviews need to be slow / careful, but I am definitely interested in getting this issue resolved!

[fandreuz]

I'd like to discuss the test here. I would expect a cumulative sum instrument to aggregate all occurrences with empty attributes. Thus, I would expect to see 6 here instead of 3.

Yes I believe you're correct. The test / logic seem appear to be incorrect currently

Oh I forgot about a fundamental detail of asynchronous instruments: conceptually, when you make an observation with a cumulative instrument, we say that you are observing a cumulative value. Thus, if we have a reader with cumulative temporality which reads twice and invokes the callbacks twice, and the callback records the same value 3 each time, the reported value should indeed be 3 and not 6.

Consider it from the perspective of this instrumentation which records information about classes loaded. If in sequential reads the callback records that 100 classes have been loaded, the output of a cumulative reader should always be 100, not N*100 (where N is the number of times the callback has been invoked). Instrumentation is not responsible for managing state and recording only the delta.

Thanks for the clarification, I fixed the test accordingly

[fandreuz]

Hi @jack-berg, are the test failure something I should be concerned of? They don't seem to be related to the diff

[jack-berg]

Hi @jack-berg, are the test failure something I should be concerned of? They don't seem to be related to the diff

The "windows-latest, 21" failure is unrelated, but the "ubunut-latest, 21" failure is: https://github.com/open-telemetry/opentelemetry-java/actions/runs/14524924371/job/40754298012?pr=7264#step:6:6005

This test codifies performance improvements we made with "reusuable data mode", and acts as a barrier to make sure we don't regress. If you compare the output of the benchmark on main to the output on this branch for the DELTA, REUSABLE_DATA, ASYNC_COUNTER case, we see an increase in B/op from 35_592 to 28_016_028. Ouch!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.90%. Comparing base (8c1e348) to head (44b9825).
Report is 10 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #7264      +/-   ##
============================================
- Coverage     90.03%   89.90%   -0.14%     
+ Complexity     6916     6893      -23     
============================================
  Files           787      785       -2     
  Lines         20864    20791      -73     
  Branches       2023     2024       +1     
============================================
- Hits          18785    18692      -93     
- Misses         1441     1460      +19     
- Partials        638      639       +1     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[fandreuz]

Hi @jack-berg, all green finally!

I had to make some changes in DoubleLastValueAggregator because I started getting too many allocation there as well.

The root cause is apparently the boxing due to the usage of AtomicReference<Double> within DoubleLastValueAggregator:

[jack-berg]

Great work. Although you forgot one thing - you've eliminated the need for Measurement and all related code. I pushed a commit which deletes it and removes 345 lines! More functionality with less code!