[Metrics API/SDK] - Pass state to async callback function. by lalitb · Pull Request #1408 · open-telemetry/opentelemetry-cpp

lalitb · 2022-05-19T04:39:13Z

Fixes #1389

Changes

Changes to pass the state to the async callback function.

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 · 2022-05-19T04:39:54Z

@markwolff - Would you mind doing a quick review of the changes as suggested in the issue. Thanks.

codecov · 2022-05-19T04:47:32Z

Codecov Report

Merging #1408 (6dca5eb) into main (5c8f476) will increase coverage by 0.38%.
The diff coverage is 6.67%.

@@            Coverage Diff             @@
##             main    #1408      +/-   ##
==========================================
+ Coverage   84.99%   85.37%   +0.38%     
==========================================
  Files         152      152              
  Lines        4350     4352       +2     
==========================================
+ Hits         3697     3715      +18     
+ Misses        653      637      -16

Impacted Files	Coverage Δ
api/include/opentelemetry/metrics/meter.h	`100.00% <ø> (ø)`
api/include/opentelemetry/metrics/noop.h	`39.14% <ø> (ø)`
sdk/include/opentelemetry/sdk/metrics/meter.h	`0.00% <0.00%> (ø)`
sdk/src/metrics/meter.cc	`6.25% <0.00%> (ø)`
...telemetry/sdk/metrics/state/async_metric_storage.h	`83.34% <100.00%> (ø)`
sdk/src/metrics/metric_reader.cc	`63.89% <0.00%> (+8.34%)`	⬆️
...metrics/export/periodic_exporting_metric_reader.cc	`77.28% <0.00%> (+34.10%)`	⬆️

…cpp into add-state-callback

reyang · 2022-05-19T06:13:14Z

@markwolff - Would you mind doing a quick review of the changes as suggested in the issue. Thanks.

s/markwolff/marcalff/

marcalff

Thanks @lalitb for the fix.

Looks good to me in general, see RegisterAsyncMetricStorage for comments.

Not sure if this should be part of this PR or not, my findings are:

(i)
docs/cpp-metrics-api-design.md appears way out of date,

(ii)
sdk/include/opentelemetry/sdk/metrics/async_instruments.h
seems to be dead code, Asynchronous<T> and subclasses are not used.

Not formally in cpp-approvers, so entering review as a comment.

Regards.

marcalff · 2022-05-19T19:53:35Z

@@ -51,20 +51,22 @@ class Meter
   * shared_ptr to that Observable Counter


Typo:
Creates a Asynchronouse --> Creates an Asynchronous

lalitb · 2022-05-20T02:27:38Z

@marcalff - Thanks for reviewing the PR, it was helpful :)
Regarding

(i)
docs/cpp-metrics-api-design.md appears way out of date,

Yes, this design is for the old implementation. The plan is to remove this doc and old code once the current implementation is mature. The current metrics design was discussed here:

https://docs.google.com/document/d/17PiDI91MQy07kkiSJ7lvlnogvMD3FV85g__WEackj1M/edit?usp=sharing

I will add this as separate PR.

(ii)
sdk/include/opentelemetry/sdk/metrics/async_instruments.h
seems to be dead code, Asynchronous and subclasses are not used.

Good observation. I am planning to remove it as part of the #1338 fix, just in case, it is required internally in that fix.

marcalff

LGTM

ThomsonTan · 2022-05-31T16:31:21Z

   */
  virtual void CreateLongObservableCounter(nostd::string_view name,
-                                           void (*callback)(ObserverResult<long> &),
+                                           void (*callback)(ObserverResult<long> &, void *),


Is this a breaking change for the alpha users?

Yes , it would be breaking change.

* install sdk config (#1273) * Bump actions/cache from 2 to 3 (#1277) * Add owent as an Approver (#1276) * add owent as reviewer * fix order * Disable benchmark action failure (#1284) * metrics exemplar round 1 (#1264) * [Metrics SDK] - fix spelling (AggregationTemporarily to AggregationTemporality) (#1288) * fix compilation error with protobuf 3.5 (#1289) * Fix span SetAttribute crash (#1283) * Synchronous Metric collection (Delta , Cumulative) (#1265) * Rename `http_client_curl` to `opentelemetry_http_client_curl` (#1301) Signed-off-by: owent <admin@owent.net> * Don't show coverage annotation for pull requests (#1304) * Implement periodic exporting metric reader (#1286) * Add `async-changes` branch to pull_request of github action (#1309) Signed-off-by: owent <admin@owent.net> * Add InstrumentationInfo and Resource to the metrics data to be exported. (#1299) * Excempt should be applied on issue instead of PR (#1316) * Bump codecov/codecov-action from 2.1.0 to 3 (#1318) * Move public definitions into `opentelemetry_api`. (#1314) Signed-off-by: owent <admin@owent.net> * Add building test without RTTI (#1294) * Remove implicitly deleted default constructor (#1267) Co-authored-by: Tom Tan <Tom.Tan@microsoft.com> Co-authored-by: Lalit Kumar Bhasin <lalit_fin@yahoo.com> * [ETW Exporter] - ETW provider handle cleanup (#1322) * Bump actions/stale from 4 to 5 (#1323) * ostream metrics example (#1312) * Prepare v1.3.0 release (#1324) * Update yield logic for ARM processor (#1325) * Fix for #1292 (#1326) * Implement Merge and Diff operation for Histogram Aggregation (#1303) * fix metrics compiler warnings (#1328) * Replace deprecated googletest API (#1327) * Remove redundant tail / in CMake install (#1329) * dependencies image as artifact (#1333) Co-authored-by: Lalit Kumar Bhasin <lalit_fin@yahoo.com> * metrics histogram example (#1330) * Link `opentelemetry_ext` with `opentelemetry_api` (#1336) * ostream metrics cmake (#1344) * prometheus exporter (#1331) * remove exporter registration to meter provider (#1350) * Bump github/codeql-action from 1 to 2 (#1351) Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Add explicit type cast in baggage UrlDecode (#1353) * Fix scalar delete against array (#1356) * conditional include for codecvt header (#1355) * Add missing include guard (#1357) * Use latest TraceLoggingDynamic.h (#1354) * prometheus example (#1332) * Fix output time in metrics OStream exporter (#1346) * Simplify SDK Configuration: Use View with default aggregation if no matching View is configured (#1358) * Fix class member initialization order (#1360) * codecov ignore (#1364) * export opentelemetry_otlp_recordable (#1365) * Disable test on prometheus-cpp which not need (#1363) * Enable metric collection for Async Instruments - Delta and Cumulative (#1334) * fix baggage propagation for empty/invalid baggage context (#1367) * Fix empty tracestate header propagation (#1373) * Bump docker/setup-qemu-action from 1 to 2 (#1375) * Add noexcept/const qualifier at missing places for Trace API. (#1374) * fix noxcept * fix etw * Bump docker/build-push-action from 2 to 3 (#1377) * Bump docker/setup-buildx-action from 1 to 2 (#1376) * [Metrics SDK] Remove un-necessary files. (#1379) * reuse temporal metric storage for sync storage (#1369) * Prometheus exporter meters and instrument name (#1378) * Fix sharing resource in batched exported spans (#1386) Co-authored-by: Lalit Kumar Bhasin <lalit_fin@yahoo.com> * fix: missing link to nlohmann_json (#1390) * Getting started document using ostream exporter (#1394) Co-authored-by: Reiley Yang <reyang@microsoft.com> * Connect async storage with async instruments (#1388) * get span_id from context when Logger::Log received invalid span_id (#1398) * Alpine image (#1382) * Upgrade proto to v0.17.0, update log data model (#1383) * Prepare v1.4.0 release (#1404) * Fix vcpkg package name in doc (#1392) * Document Getting Started with Prometheus and Grafana (#1396) Co-authored-by: Lalit Kumar Bhasin <lalit_fin@yahoo.com> * fix OTEL_INTERNAL_LOG_INFO (#1407) * fix: WaitOnSocket select error when sockfd above FD_SETSIZE (#1410) * [BUILD] fix nlohmann_json's (third party) include dir (#1415) * [Metrics API/SDK] - Pass state to async callback function. (#1408) * Copy string_view passed to ETW exporter in PropertyVariant (#1425) * Fix ETW log exporter header inclusion (#1426) * [Metrics SDK] Only record non-negative / finite / Non-NAN histogram values (#1427) * validate histogram value * handle Nan * add changelog * divide by 0 error on windows * fix markdown lint * Fix global log handle symbols when using dlopen (#1420) * Add attributes/dimensions to metrics ostream exporter (#1400) * Log current timestamp instead of epoch time (#1434) * install sdk-config.h (#1419) * Fix GettingStarted documentation for Jaeger HTTP exporter (#1347) (#1439) * fix histogram (#1440) * Upgrade nlohmann_json to 3.10.5 (#1438) (#1441) * Fixed broken link to OpenTelemetry.io (#1445) (#1446) Co-authored-by: Ehsan Saei <71217171+esigo@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Lalit Kumar Bhasin <lalit_fin@yahoo.com> Co-authored-by: Tom Tan <Tom.Tan@microsoft.com> Co-authored-by: Ben Landrum <71329856+benlandrum@users.noreply.github.com> Co-authored-by: jmanjon <67091862+juandemanjon@users.noreply.github.com> Co-authored-by: Zsolt Bölöny <bolony.zsolt@gmail.com> Co-authored-by: Leo Di Donato <leodidonato@gmail.com> Co-authored-by: Reiley Yang <reyang@microsoft.com> Co-authored-by: Lalit Kumar Bhasin <labhas@microsoft.com> Co-authored-by: Hamed Mansouri <hamed0381@gmail.com> Co-authored-by: ztao <t@taozj.org> Co-authored-by: Flier Lu <flier@users.noreply.github.com> Co-authored-by: Marc Alff <marc.alff@free.fr> Co-authored-by: Marc Alff <marc.alff@oracle.com>

* install sdk config (#1273) * Bump actions/cache from 2 to 3 (#1277) * Add owent as an Approver (#1276) * add owent as reviewer * fix order * Disable benchmark action failure (#1284) * metrics exemplar round 1 (#1264) * [Metrics SDK] - fix spelling (AggregationTemporarily to AggregationTemporality) (#1288) * fix compilation error with protobuf 3.5 (#1289) * Fix span SetAttribute crash (#1283) * Synchronous Metric collection (Delta , Cumulative) (#1265) * Rename `http_client_curl` to `opentelemetry_http_client_curl` (#1301) Signed-off-by: owent <admin@owent.net> * Don't show coverage annotation for pull requests (#1304) * Implement periodic exporting metric reader (#1286) * Add `async-changes` branch to pull_request of github action (#1309) Signed-off-by: owent <admin@owent.net> * Add InstrumentationInfo and Resource to the metrics data to be exported. (#1299) * Excempt should be applied on issue instead of PR (#1316) * Bump codecov/codecov-action from 2.1.0 to 3 (#1318) * Move public definitions into `opentelemetry_api`. (#1314) Signed-off-by: owent <admin@owent.net> * Add building test without RTTI (#1294) * Remove implicitly deleted default constructor (#1267) Co-authored-by: Tom Tan <Tom.Tan@microsoft.com> Co-authored-by: Lalit Kumar Bhasin <lalit_fin@yahoo.com> * [ETW Exporter] - ETW provider handle cleanup (#1322) * Bump actions/stale from 4 to 5 (#1323) * ostream metrics example (#1312) * Prepare v1.3.0 release (#1324) * Update yield logic for ARM processor (#1325) * Fix for #1292 (#1326) * Implement Merge and Diff operation for Histogram Aggregation (#1303) * fix metrics compiler warnings (#1328) * Replace deprecated googletest API (#1327) * Remove redundant tail / in CMake install (#1329) * dependencies image as artifact (#1333) Co-authored-by: Lalit Kumar Bhasin <lalit_fin@yahoo.com> * metrics histogram example (#1330) * Link `opentelemetry_ext` with `opentelemetry_api` (#1336) * ostream metrics cmake (#1344) * prometheus exporter (#1331) * remove exporter registration to meter provider (#1350) * Bump github/codeql-action from 1 to 2 (#1351) Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Add explicit type cast in baggage UrlDecode (#1353) * Fix scalar delete against array (#1356) * conditional include for codecvt header (#1355) * Add missing include guard (#1357) * Use latest TraceLoggingDynamic.h (#1354) * prometheus example (#1332) * Fix output time in metrics OStream exporter (#1346) * Simplify SDK Configuration: Use View with default aggregation if no matching View is configured (#1358) * Fix class member initialization order (#1360) * codecov ignore (#1364) * export opentelemetry_otlp_recordable (#1365) * Disable test on prometheus-cpp which not need (#1363) * Enable metric collection for Async Instruments - Delta and Cumulative (#1334) * fix baggage propagation for empty/invalid baggage context (#1367) * Fix empty tracestate header propagation (#1373) * Bump docker/setup-qemu-action from 1 to 2 (#1375) * Add noexcept/const qualifier at missing places for Trace API. (#1374) * fix noxcept * fix etw * Bump docker/build-push-action from 2 to 3 (#1377) * Bump docker/setup-buildx-action from 1 to 2 (#1376) * [Metrics SDK] Remove un-necessary files. (#1379) * reuse temporal metric storage for sync storage (#1369) * Prometheus exporter meters and instrument name (#1378) * Fix sharing resource in batched exported spans (#1386) Co-authored-by: Lalit Kumar Bhasin <lalit_fin@yahoo.com> * fix: missing link to nlohmann_json (#1390) * Getting started document using ostream exporter (#1394) Co-authored-by: Reiley Yang <reyang@microsoft.com> * Connect async storage with async instruments (#1388) * get span_id from context when Logger::Log received invalid span_id (#1398) * Alpine image (#1382) * Upgrade proto to v0.17.0, update log data model (#1383) * Prepare v1.4.0 release (#1404) * Fix vcpkg package name in doc (#1392) * Document Getting Started with Prometheus and Grafana (#1396) Co-authored-by: Lalit Kumar Bhasin <lalit_fin@yahoo.com> * fix OTEL_INTERNAL_LOG_INFO (#1407) * fix: WaitOnSocket select error when sockfd above FD_SETSIZE (#1410) * [BUILD] fix nlohmann_json's (third party) include dir (#1415) * [Metrics API/SDK] - Pass state to async callback function. (#1408) * Copy string_view passed to ETW exporter in PropertyVariant (#1425) * Fix ETW log exporter header inclusion (#1426) * [Metrics SDK] Only record non-negative / finite / Non-NAN histogram values (#1427) * validate histogram value * handle Nan * add changelog * divide by 0 error on windows * fix markdown lint * Fix global log handle symbols when using dlopen (#1420) * Add attributes/dimensions to metrics ostream exporter (#1400) * Log current timestamp instead of epoch time (#1434) * install sdk-config.h (#1419) * Fix GettingStarted documentation for Jaeger HTTP exporter (#1347) (#1439) * fix histogram (#1440) * Upgrade nlohmann_json to 3.10.5 (#1438) (#1441) * Fixed broken link to OpenTelemetry.io (#1445) (#1446) * Fix variables inizialization (#1430) * Remove `AsyncBatch*Processor`, implement [opentelemetry-specification#2452](open-telemetry/opentelemetry-specification#2452) Signed-off-by: WenTao Ou <admin@owent.net> * Remove invalid changelog Signed-off-by: WenTao Ou <admin@owent.net> * Fix compiling problems without `ENABLE_ASYNC_EXPORT` Signed-off-by: WenTao Ou <admin@owent.net> * Always return `ExportResult::kSuccess` Signed-off-by: WenTao Ou <admin@owent.net> Co-authored-by: Ehsan Saei <71217171+esigo@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Lalit Kumar Bhasin <lalit_fin@yahoo.com> Co-authored-by: Tom Tan <Tom.Tan@microsoft.com> Co-authored-by: Ben Landrum <71329856+benlandrum@users.noreply.github.com> Co-authored-by: jmanjon <67091862+juandemanjon@users.noreply.github.com> Co-authored-by: Zsolt Bölöny <bolony.zsolt@gmail.com> Co-authored-by: Leo Di Donato <leodidonato@gmail.com> Co-authored-by: Reiley Yang <reyang@microsoft.com> Co-authored-by: Lalit Kumar Bhasin <labhas@microsoft.com> Co-authored-by: Hamed Mansouri <hamed0381@gmail.com> Co-authored-by: ztao <t@taozj.org> Co-authored-by: Flier Lu <flier@users.noreply.github.com> Co-authored-by: Marc Alff <marc.alff@free.fr> Co-authored-by: Marc Alff <marc.alff@oracle.com> Co-authored-by: univisionsrl <31103417+univisionsrl@users.noreply.github.com>

lalitb and others added 2 commits May 18, 2022 17:26

initial commit

2feef1b

more changes

eaca516

lalitb requested a review from a team May 19, 2022 04:39

Merge branch 'main' into add-state-callback

f22dcb8

lalitb added 2 commits May 18, 2022 22:17

comment out argument not used

d9ec874

Merge branch 'add-state-callback' of github.com:lalitb/opentelemetry-…

31cfc04

…cpp into add-state-callback

marcalff reviewed May 19, 2022

View reviewed changes

fix review comments

824ebc0

marcalff approved these changes May 20, 2022

View reviewed changes

Merge branch 'main' into add-state-callback

154afa5

ThomsonTan reviewed May 31, 2022

View reviewed changes

ThomsonTan approved these changes May 31, 2022

View reviewed changes

Merge branch 'main' into add-state-callback

6dca5eb

lalitb merged commit 9df5c4f into open-telemetry:main May 31, 2022

marcalff mentioned this pull request Aug 2, 2022

REQUEST: New membership for @marcalff open-telemetry/community#1122

Closed

6 tasks

Navigation Menu

Uh oh!

[Metrics API/SDK] - Pass state to async callback function. - #1408

[Metrics API/SDK] - Pass state to async callback function.#1408
lalitb merged 8 commits into
open-telemetry:mainfrom
lalitb:add-state-callback

lalitb commented May 19, 2022

Uh oh!

lalitb commented May 19, 2022 •

edited

Loading

Uh oh!

codecov Bot commented May 19, 2022 •

edited

Loading

Uh oh!

reyang commented May 19, 2022

Uh oh!

marcalff left a comment

Uh oh!

Uh oh!

marcalff May 19, 2022

Uh oh!

lalitb May 20, 2022

Uh oh!

lalitb commented May 20, 2022

Uh oh!

marcalff left a comment

Uh oh!

ThomsonTan May 31, 2022

Uh oh!

lalitb May 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		@@ -51,20 +51,22 @@ class Meter
		* shared_ptr to that Observable Counter

Uh oh!

Conversation

lalitb commented May 19, 2022

Changes

Uh oh!

lalitb commented May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

reyang commented May 19, 2022

Uh oh!

marcalff left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marcalff May 19, 2022

Choose a reason for hiding this comment

Uh oh!

lalitb May 20, 2022

Choose a reason for hiding this comment

Uh oh!

lalitb commented May 20, 2022

Uh oh!

marcalff left a comment

Choose a reason for hiding this comment

Uh oh!

ThomsonTan May 31, 2022

Choose a reason for hiding this comment

Uh oh!

lalitb May 31, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lalitb commented May 19, 2022 •

edited

Loading

codecov Bot commented May 19, 2022 •

edited

Loading