metrics exemplar round 1 by esigo · Pull Request #1264 · open-telemetry/opentelemetry-cpp

esigo · 2022-03-13T20:19:17Z

Fixes #1093 (issue)

Changes

Adds ExemplarFilter and ExemplarReservoir
missing:
Filtered, fixed size, and histogram exemplar reservoir.

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

codecov · 2022-03-13T20:26:55Z

Codecov Report

Merging #1264 (8f6d0a3) into main (0c9aece) will increase coverage by 0.05%.
The diff coverage is 96.03%.

@@            Coverage Diff             @@
##             main    #1264      +/-   ##
==========================================
+ Coverage   91.99%   92.03%   +0.05%     
==========================================
  Files         205      213       +8     
  Lines        7395     7510     +115     
==========================================
+ Hits         6802     6911     +109     
- Misses        593      599       +6

Impacted Files	Coverage Δ
...e/opentelemetry/sdk/metrics/state/metric_storage.h	`100.00% <ø> (ø)`
...clude/opentelemetry/sdk/metrics/sync_instruments.h	`100.00% <ø> (ø)`
...ntelemetry/sdk/metrics/view/attributes_processor.h	`100.00% <ø> (ø)`
sdk/src/metrics/meter.cc	`7.80% <0.00%> (-0.10%)`	⬇️
...ntelemetry/sdk/metrics/state/sync_metric_storage.h	`66.67% <72.73%> (-1.07%)`	⬇️
...telemetry/sdk/metrics/state/multi_metric_storage.h	`88.24% <75.00%> (ø)`
api/include/opentelemetry/metrics/noop.h	`34.62% <100.00%> (+5.73%)`	⬆️
...i/include/opentelemetry/metrics/sync_instruments.h	`100.00% <100.00%> (ø)`
api/test/metrics/noop_sync_instrument_test.cc	`100.00% <100.00%> (ø)`
...emetry/sdk/metrics/exemplar/always_sample_filter.h	`100.00% <100.00%> (ø)`
... and 12 more

lalitb · 2022-03-21T17:32:48Z

+    this->Add(value,
+              nostd::span<const std::pair<nostd::string_view, common::AttributeValue>>{
+                  attributes.begin(), attributes.end()},
+              context);


Should we make context optional, and use default runtime context if not provided.

void Add(value); void Add(value, context); void Add(value, attributes); void Add(value, attributes, context);

or just

void Add(value) void Add(value, attributes) void Add(value, attributes, context)

thanks, added these four:

void Add(value); void Add(value, context); void Add(value, attributes); void Add(value, attributes, context);

lalitb · 2022-03-21T17:40:03Z

+{
+public:
+  // Returns whether or not a reservoir should attempt to filter a measurement.
+  virtual bool shouldSampleMeasurement(long value,


ShouldSampleMeasurement ?

thanks done.

lalitb · 2022-03-21T17:43:53Z

+public:
+  static nostd::shared_ptr<ExemplarFilter> GetAlwaysSampleFilter()
+  {
+    return nostd::shared_ptr<ExemplarFilter>{new AlwaysSampleFilter{}};


nit - using c++11 static magic to ensure we always use one shared_ptr instance?

thanks, done

lalitb

LGTM with few minor comments.

lalitb · 2022-03-22T06:08:36Z

+  virtual ~ExemplarReservoir() = default;
+
+  /** Offers a long measurement to be sampled. */
+  virtual void OfferMeasurement(long value,


nit - Should this method also accept the time when the measurement was taken (as per the specs ) : https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#exemplarreservoir

Thanks added timestamp.

lalitb · 2022-03-22T06:11:33Z

+                          const opentelemetry::common::KeyValueIterable &attributes,
+                          const opentelemetry::context::Context &context) noexcept override
  {
+    exemplar_reservoir_->OfferMeasurement(value, attributes, context);


Should this method be called in this storage wrapper class, or just inside all the storage?

removed from the MultiMetricStorage.

lalitb · 2022-03-22T06:13:33Z

+  MetricAttributes GetFilteredAttributes();
+
+  /** Returns the timestamp in nanos when measurement was collected. */
+  long GetEpochNanos();


nit - timestamp is represented as sdk::common::SystemTimestamp at other places.

Thanks, done.

reyang

Need to rename "examplar" to "exemplar".

lalitb · 2022-03-22T07:19:44Z

Need to rename "examplar" to "exemplar".

Good observation :)

…etry-cpp into metrics-examplar

esigo · 2022-03-22T19:48:24Z

Need to rename "examplar" to "exemplar".

Good catch, thanks.
Fixed the typo.

reyang

LGTM for round1.

* 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) * Fix conflicts Signed-off-by: owent <admin@owent.net> * Using clang-format-10 to format codes(clang-format-14 has a different output) Signed-off-by: owent <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>

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

esigo and others added 3 commits March 13, 2022 20:15

metrics examplar

fa52139

CI branch

f8b4b2c

Merge branch 'main' into metrics-examplar

d715cf6

esigo and others added 4 commits March 18, 2022 18:03

Merge branch 'main' into metrics-examplar

60fedc8

format

d41d5bd

fix CI

4dbf6e1

fix CI

c515b57

esigo changed the title ~~[WIP] metrics examplar~~ metrics examplar round 1 Mar 18, 2022

esigo marked this pull request as ready for review March 18, 2022 20:29

esigo requested a review from a team March 18, 2022 20:29

Merge branch 'main' into metrics-examplar

c8d6f40

lalitb reviewed Mar 21, 2022

View reviewed changes

esigo added 2 commits March 21, 2022 18:45

comments

bda900d

sync_instrument bazel

e3349c8

lalitb approved these changes Mar 22, 2022

View reviewed changes

reyang requested changes Mar 22, 2022

View reviewed changes

esigo and others added 4 commits March 22, 2022 19:36

comments

1e5a47d

Merge branch 'main' into metrics-examplar

76db5a7

fix CI

5ba7f15

Merge branch 'metrics-examplar' of https://github.com/esigo/opentelem…

2d1bd93

…etry-cpp into metrics-examplar

esigo changed the title ~~metrics examplar round 1~~ metrics exemplar round 1 Mar 22, 2022

reyang approved these changes Mar 22, 2022

View reviewed changes

lalitb and others added 2 commits March 23, 2022 09:56

Merge branch 'main' into metrics-examplar

f607830

Merge branch 'main' into metrics-examplar

8f6d0a3

esigo merged commit 3c7b44b into open-telemetry:main Mar 24, 2022

esigo deleted the metrics-examplar branch March 24, 2022 21:31

Uh oh!

Conversation

esigo commented Mar 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lalitb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reyang left a comment

Choose a reason for hiding this comment

Uh oh!

lalitb commented Mar 22, 2022

Uh oh!

esigo commented Mar 22, 2022

Uh oh!

reyang left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

esigo commented Mar 13, 2022 •

edited

Loading

codecov Bot commented Mar 13, 2022 •

edited

Loading

lalitb Mar 21, 2022 •

edited

Loading

lalitb Mar 22, 2022 •

edited

Loading