Skip to content

Add logic to refrain from exporting un-updated metric instruments#293

Merged
reyang merged 13 commits into
open-telemetry:masterfrom
Brandon-Kimberly:master
Aug 24, 2020
Merged

Add logic to refrain from exporting un-updated metric instruments#293
reyang merged 13 commits into
open-telemetry:masterfrom
Brandon-Kimberly:master

Conversation

@Brandon-Kimberly
Copy link
Copy Markdown
Contributor

This PR is meant to resolve Issue #255.

I have added a new bool variable to the aggregator base class to track whether or not the aggregator has been updated in the most recent collection cycle. Some additional logic was then added to the SDK sync instrument classes to refrain from creating records from aggregators that have received no recent updates. Since there are no records created from these un-updated (stale) instruments, they will not be exported.

I did not implement the same logic for the SDK async instrument classes as I don't believe it would make sense there. Asynchronous instruments are supposed to be exported by interval rather than by request: Therefore they are never stale.

@Brandon-Kimberly Brandon-Kimberly requested a review from a team August 23, 2020 17:54
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 23, 2020

Codecov Report

Merging #293 into master will increase coverage by 0.50%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #293      +/-   ##
==========================================
+ Coverage   93.93%   94.43%   +0.50%     
==========================================
  Files         146      146              
  Lines        6564     6597      +33     
==========================================
+ Hits         6166     6230      +64     
+ Misses        398      367      -31     
Impacted Files Coverage Δ
.../opentelemetry/sdk/metrics/aggregator/aggregator.h 58.82% <100.00%> (+2.57%) ⬆️
...emetry/sdk/metrics/aggregator/counter_aggregator.h 96.15% <100.00%> (+0.32%) ⬆️
...elemetry/sdk/metrics/aggregator/exact_aggregator.h 97.56% <100.00%> (+0.12%) ⬆️
...elemetry/sdk/metrics/aggregator/gauge_aggregator.h 100.00% <100.00%> (ø)
...etry/sdk/metrics/aggregator/histogram_aggregator.h 96.55% <100.00%> (+0.12%) ⬆️
.../metrics/aggregator/min_max_sum_count_aggregator.h 100.00% <100.00%> (ø)
...lemetry/sdk/metrics/aggregator/sketch_aggregator.h 69.76% <100.00%> (+0.71%) ⬆️
...clude/opentelemetry/sdk/metrics/sync_instruments.h 94.77% <100.00%> (+23.78%) ⬆️
sdk/test/metrics/metric_instrument_test.cc 100.00% <100.00%> (ø)

Copy link
Copy Markdown
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

@reyang reyang added the pr:please-merge This PR is ready to be merged by a Maintainer (rebased, CI passed, has enough valid approvals, etc.) label Aug 24, 2020
@reyang reyang merged commit 4710d4b into open-telemetry:master Aug 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr:please-merge This PR is ready to be merged by a Maintainer (rebased, CI passed, has enough valid approvals, etc.)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants