Skip to content

feat(sdk-metrics-base): detect resets on async metrics - #2990

Merged
legendecas merged 3 commits into
open-telemetry:mainfrom
legendecas:metrics-ff/monotonicity
Jun 28, 2022
Merged

feat(sdk-metrics-base): detect resets on async metrics#2990
legendecas merged 3 commits into
open-telemetry:mainfrom
legendecas:metrics-ff/monotonicity

Conversation

@legendecas

@legendecas legendecas commented May 23, 2022

Copy link
Copy Markdown
Member

Which problem is this PR solving?

Detect resets on async metrics.

Fixes #2987

Short description of the changes

  • Add monotonicity support in SumAggregator
    • If the SumAggregator is monotonic, it performs SumAggregator.diff and returns the current value if the previous value is greater than the current one.
    • Negative values are ignored in monotonic SumAccumulation.
    • SumAggregator.merge distinguishes a reset delta, and does not merge it with previous accumulation.
  • Added reset and gaps detection for async metric instruments.
    • CUMULATIVE reader now collected the reset value and startTime.
    • Fixed a problem that DELTA reader may receive negative values on resetting a monotonic metric.
  • Resets on async metrics also resets the data points' exported start time.
  • The startTime and endTime now follow the spec definitions (reset & gaps):
    • For points with delta aggregation temporality, the startTime of each point matches the endTime of the preceding point (for that particular reader).
    • Otherwise, the startTime of each point matches the startTime of the initial observation.

For an event stream, we should provide the following metric exports:

Async Monotonic Event Inputs Cumulative Exports Delta Exports
1, Event Time T1 1, StartTime T1, EndTime T1 1, T1-T1
100, T2 100, T1-T2 99, T1-T2
200, T3 200, T1-T3 100, T2-T3
1, T4 (Reset) 1, T4-T4 1, T4-T4
200, T5 200, T4-T5 199, T4-T5
(A gap, no inputs), T6 200, T4-T6 (no output)
300, T7 300, T4-T7 100, T7-T7
Async Non-monotonic Event Inputs Cumulative Exports Delta Exports
1, Event Time T1 1, StartTime T1, EndTime T1 1, T1-T1
100, T2 100, T1-T2 99, T1-T2
200, T3 200, T1-T3 100, T2-T3
1, T4 1, T1-T4 -199, T3-T4
200, T5 200, T1-T5 199, T4-T5
(A gap, no inputs), T6 200, T1-T6 (no output)
300, T7 300, T1-T7 100, T7-T7

For a sync event stream, the exported times are:

Sync Event Inputs Cumulative Exports Delta Exports
100, Event Time T1 (not exporting) (not exporting)
(No event), T2 100, T1-T2 100, T1-T2
100, T3 (not exporting) (not exporting)
100, T4 (not exporting) (not exporting)
(No event), T5 300, T1-T5 200, T2-T5
(No event), T6 300, T1-T6 (no output)

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • Monotonic SumAggregator and Non-monotonic SumAggregator
  • Add checks on SyncMetricStorage exported startTime and endTime
  • Add checks on AsyncMetricStorage exported startTime and endTime

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added

@codecov

codecov Bot commented May 23, 2022

Copy link
Copy Markdown

Codecov Report

Merging #2990 (40d19c3) into main (4378303) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2990      +/-   ##
==========================================
- Coverage   93.10%   93.08%   -0.03%     
==========================================
  Files         188      188              
  Lines        6224     6261      +37     
  Branches     1316     1323       +7     
==========================================
+ Hits         5795     5828      +33     
- Misses        429      433       +4     
Impacted Files Coverage Δ
...ntelemetry-sdk-metrics-base/src/aggregator/Drop.ts 100.00% <ø> (ø)
...telemetry-sdk-metrics-base/src/aggregator/types.ts 100.00% <ø> (ø)
...metrics-base/src/state/MeterProviderSharedState.ts 100.00% <ø> (ø)
...emetry-sdk-metrics-base/src/state/MetricStorage.ts 100.00% <ø> (ø)
.../opentelemetry-sdk-metrics-base/src/Instruments.ts 94.73% <100.00%> (+0.14%) ⬆️
...metry-sdk-metrics-base/src/aggregator/Histogram.ts 91.04% <100.00%> (-5.84%) ⬇️
...metry-sdk-metrics-base/src/aggregator/LastValue.ts 100.00% <100.00%> (ø)
...entelemetry-sdk-metrics-base/src/aggregator/Sum.ts 100.00% <100.00%> (ø)
...y-sdk-metrics-base/src/state/AsyncMetricStorage.ts 100.00% <100.00%> (ø)
...sdk-metrics-base/src/state/DeltaMetricProcessor.ts 100.00% <100.00%> (ø)
... and 8 more

@legendecas
legendecas force-pushed the metrics-ff/monotonicity branch 3 times, most recently from 5769113 to 4b8448d Compare May 25, 2022 08:11
@legendecas legendecas changed the title feat(sdk-metrics-base): sum aggregator monotonicity feat(sdk-metrics-base): detect resets on async metrics May 25, 2022
@legendecas
legendecas force-pushed the metrics-ff/monotonicity branch 11 times, most recently from 45c7acc to 672368f Compare May 31, 2022 03:04
@legendecas
legendecas force-pushed the metrics-ff/monotonicity branch from 672368f to ec2a9da Compare May 31, 2022 06:53
@legendecas
legendecas marked this pull request as ready for review May 31, 2022 07:32
@legendecas
legendecas requested a review from a team May 31, 2022 07:32
@dyladan dyladan added this to the Metrics GA milestone Jun 1, 2022

@dyladan dyladan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry for late review. Large PR is hard to get time to really go through. Most changes are just passing the hrtime though. I added some comments but none of them are blocking.

Comment thread experimental/packages/opentelemetry-sdk-metrics-base/src/aggregator/Sum.ts Outdated

@pichlermarc pichlermarc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Finally had the time to look through this. 🙂
Just a few non-blocking (mostly docs related) comments. Thanks for taking the time to work on this! 🙂

Comment thread experimental/packages/opentelemetry-sdk-metrics-base/test/aggregator/Sum.test.ts Outdated
…city

# Conflicts:
#	experimental/packages/opentelemetry-sdk-metrics-base/src/aggregator/Histogram.ts
#	experimental/packages/opentelemetry-sdk-metrics-base/test/aggregator/Histogram.test.ts
@legendecas
legendecas merged commit 6eca6d4 into open-telemetry:main Jun 28, 2022
@legendecas
legendecas deleted the metrics-ff/monotonicity branch June 28, 2022 17:42
@legendecas

Copy link
Copy Markdown
Member Author

Thank you for taking a look at this!

pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Jul 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Asynchronous instrument gaps and resets on delta temporality

3 participants