-
Notifications
You must be signed in to change notification settings - Fork 807
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
feat(sdk-metrics-base): detect resets on async metrics #2990
Conversation
Codecov Report
@@ 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
|
5769113
to
4b8448d
Compare
45c7acc
to
672368f
Compare
672368f
to
ec2a9da
Compare
There was a problem hiding this 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.
experimental/packages/opentelemetry-sdk-metrics-base/src/state/TemporalMetricProcessor.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/src/state/TemporalMetricProcessor.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/src/aggregator/Sum.ts
Show resolved
Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/src/aggregator/Sum.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/src/state/DeltaMetricProcessor.ts
Show resolved
Hide resolved
There was a problem hiding this 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! 🙂
experimental/packages/opentelemetry-sdk-metrics-base/src/state/TemporalMetricProcessor.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/src/state/TemporalMetricProcessor.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/src/state/DeltaMetricProcessor.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/src/state/DeltaMetricProcessor.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/test/aggregator/Sum.test.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/src/aggregator/Sum.ts
Show resolved
Hide resolved
…city # Conflicts: # experimental/packages/opentelemetry-sdk-metrics-base/src/aggregator/Histogram.ts # experimental/packages/opentelemetry-sdk-metrics-base/test/aggregator/Histogram.test.ts
Thank you for taking a look at this! |
Which problem is this PR solving?
Detect resets on async metrics.
Fixes #2987
Short description of the changes
SumAggregator.diff
and returns the current value if the previous value is greater than the current one.SumAggregator.merge
distinguishes a reset delta, and does not merge it with previous accumulation.startTime
andendTime
now follow the spec definitions (reset & gaps):startTime
of each point matches theendTime
of the preceding point (for that particular reader).startTime
of each point matches thestartTime
of the initial observation.For an event stream, we should provide the following metric exports:
For a sync event stream, the exported times are:
Type of change
How Has This Been Tested?
Checklist: