Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Aggregation Temporality Support #1730

Closed

Conversation

legendecas
Copy link
Member

Which problem is this PR solving?

Short description of the changes

  • Make metrics and bound metrics state ephemera, so that a label set will not be kept in store throughout the process lifetime without manual unbind.
  • Rename UngroupedProcessor to BasicProcessor and supports cumulative processing to aggregate AggregationTemporality.DELTA aggregator snapshots to AggregationTemporality.CUMULATIVE accumulations.
  • Add move and merge to Aggregator interfaces.

This PR depends on previous minor type introductions:

instrument.update(value);
}
// greater or equal then previous value.
// However it is impractical for the stateless accumulator to remember
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the aggregator in the Accumulator (class Metric in the context) no longer bookkeeping all the records of past label-aggregator pairs, it is impractical for SumObserver to determine if it is updating with a rising value (unless the observer itself memos last value, but I'm not favoring the workaround). Some practices may recognize the value drop as a reset and correctly reflect the true trend, So I'd believe the value drop in SumObserver can reflect the true trends rather than discarding data frames.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SumObserver cannot have value drop, this is monotonic metric - it can only raise or remain the same.

Copy link
Member Author

@legendecas legendecas Dec 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you are right. I've checked again the status quo PR of the processor specs, SumObserver and
UpDownSumObserver instruments are configured for Cumulative aggregation temporality in StatelessAggregationTemporalitySelector .

@codecov
Copy link

codecov bot commented Dec 8, 2020

Codecov Report

Merging #1730 (fa3617c) into master (781b30f) will decrease coverage by 0.46%.
The diff coverage is 80.54%.

@@            Coverage Diff             @@
##           master    #1730      +/-   ##
==========================================
- Coverage   91.38%   90.91%   -0.47%     
==========================================
  Files         164      165       +1     
  Lines        5059     5153      +94     
  Branches     1048     1070      +22     
==========================================
+ Hits         4623     4685      +62     
- Misses        436      468      +32     
Impacted Files Coverage Δ
...ages/opentelemetry-metrics/src/export/Processor.ts 100.00% <ø> (+8.00%) ⬆️
...emetry-metrics/src/export/aggregators/Histogram.ts 76.19% <47.36%> (-23.81%) ⬇️
...s/opentelemetry-metrics/src/BatchObserverMetric.ts 92.85% <50.00%> (-0.25%) ⬇️
packages/opentelemetry-metrics/src/Utils.ts 68.75% <59.09%> (-21.25%) ⬇️
...emetry-metrics/src/export/aggregators/LastValue.ts 79.16% <61.53%> (-20.84%) ⬇️
...opentelemetry-metrics/src/export/BasicProcessor.ts 80.55% <80.55%> (ø)
...kages/opentelemetry-metrics/src/BoundInstrument.ts 94.73% <87.50%> (-5.27%) ⬇️
...ges/opentelemetry-metrics/src/export/Controller.ts 80.76% <88.88%> (+1.60%) ⬆️
...pentelemetry-metrics/src/export/aggregators/Sum.ts 95.83% <92.30%> (-4.17%) ⬇️
...es/opentelemetry-metrics/src/BaseObserverMetric.ts 100.00% <100.00%> (ø)
... and 10 more

@legendecas legendecas self-assigned this Dec 8, 2020
Base automatically changed from master to main January 25, 2021 19:26
@legendecas
Copy link
Member Author

Closed due to inactivity. I will continue the investigation once there are any updates on the spec.

@legendecas legendecas closed this Jun 24, 2021
@legendecas legendecas deleted the accumulator-processor branch September 27, 2021 02:27
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants