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

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

Merged
merged 3 commits into from
Jun 28, 2022

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented May 23, 2022

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
Copy link

codecov bot commented May 23, 2022

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 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
Copy link
Member

@dyladan dyladan left a comment

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.

Copy link
Member

@pichlermarc pichlermarc left a comment

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! 🙂

…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
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