Skip to content

Support multiple async callbacks - #1495

Merged
lalitb merged 32 commits into
open-telemetry:mainfrom
lalitb:multi-callback-approach
Aug 5, 2022
Merged

Support multiple async callbacks#1495
lalitb merged 32 commits into
open-telemetry:mainfrom
lalitb:multi-callback-approach

Conversation

@lalitb

@lalitb lalitb commented Jul 13, 2022

Copy link
Copy Markdown
Member

Fixes #1338

Changes

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@lalitb
lalitb requested a review from a team July 13, 2022 21:21
@lalitb lalitb changed the title [WIP] Support multiple async callbacks Support multiple async callbacks Jul 25, 2022
@codecov

codecov Bot commented Jul 25, 2022

Copy link
Copy Markdown

Codecov Report

Merging #1495 (f4b6b37) into main (d299348) will decrease coverage by 0.86%.
The diff coverage is 25.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1495      +/-   ##
==========================================
- Coverage   84.57%   83.72%   -0.85%     
==========================================
  Files         156      156              
  Lines        4795     4850      +55     
==========================================
+ Hits         4055     4060       +5     
- Misses        740      790      +50     
Impacted Files Coverage Δ
api/include/opentelemetry/metrics/meter.h 100.00% <ø> (ø)
api/include/opentelemetry/metrics/noop.h 36.00% <0.00%> (-3.13%) ⬇️
...dk/include/opentelemetry/sdk/metrics/instruments.h 0.00% <0.00%> (ø)
...ntelemetry/sdk/metrics/state/sync_metric_storage.h 64.71% <ø> (ø)
...clude/opentelemetry/sdk/metrics/sync_instruments.h 100.00% <ø> (ø)
sdk/src/metrics/meter.cc 5.27% <0.00%> (-1.05%) ⬇️
sdk/src/metrics/state/observable_registry.cc 15.16% <15.16%> (ø)
...telemetry/sdk/metrics/state/multi_metric_storage.h 56.67% <21.43%> (-31.56%) ⬇️
sdk/src/metrics/async_instruments.cc 53.34% <53.34%> (ø)
...telemetry/sdk/metrics/state/async_metric_storage.h 85.30% <73.69%> (+1.97%) ⬆️
... and 9 more

@esigo esigo added the metrics label Jul 31, 2022

@esigo esigo 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.

Thanks for the RP.
I had some minor comments.

Comment thread sdk/test/metrics/async_metric_storage_test.cc Outdated
Comment thread api/include/opentelemetry/metrics/async_instruments.h Outdated
Comment thread sdk/test/metrics/async_instruments_test.cc Outdated
Comment thread sdk/src/metrics/state/observable_registry.cc
Comment thread sdk/src/metrics/state/observable_registry.cc Outdated
Comment thread sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h Outdated
Comment thread sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h Outdated
Comment thread sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h Outdated
Comment thread sdk/src/metrics/meter.cc
Comment thread examples/common/metrics_foo_library/foo_library.cc

@esigo esigo 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.

LGTM
Thanks for the PR :)

@lalitb
lalitb merged commit 73a51cf into open-telemetry:main Aug 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Metrics API/SDK] Support registering multiple async callbacks to one single async instrument.

2 participants