feat: Export MinMaxLastSumCountAggregator metrics to the collector as Summary by davidwitten · Pull Request #1320 · open-telemetry/opentelemetry-js

davidwitten · 2020-07-16T15:35:52Z

This references issue #1294.

After discussing with OpenTelemetry Go, they use the SummaryDataPoint whenever the MinMaxLastSumCountAggregator is used. They set the 0th percentile as the min, and the 100th percentile as the max.

So, I added the MetricDescriptorType Summary and created a function to add a Summary Point to the collector.

I also changed the testing code to return test metrics from a function rather than being constants, because I was running into an issue that the metrics were just being shallow-copied and different tests were affecting one another.

codecov · 2020-07-16T15:59:44Z

Codecov Report

Merging #1320 into master will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1320      +/-   ##
==========================================
+ Coverage   93.72%   93.77%   +0.05%     
==========================================
  Files         149      149              
  Lines        4317     4322       +5     
  Branches      880      880              
==========================================
+ Hits         4046     4053       +7     
+ Misses        271      269       -2

Impacted Files	Coverage Δ
...lemetry-exporter-collector/src/transformMetrics.ts	`83.87% <100.00%> (+3.18%)`	⬆️

dyladan

Looks good thanks!

packages/opentelemetry-exporter-collector/src/transformMetrics.ts

obecny

lgtm, just few minor things

packages/opentelemetry-exporter-collector/src/transformMetrics.ts

packages/opentelemetry-exporter-collector/test/helper.ts

packages/opentelemetry-exporter-collector/src/transformMetrics.ts

dyladan · 2020-08-04T14:16:47Z

@open-telemetry/javascript-approvers This has been around a while, please review so it may be merged

davidwitten · 2020-08-04T19:27:57Z

Fixed a flaky test in sdk-node

dyladan · 2020-08-04T20:07:22Z

@davidwitten how does that fix the flaky test? It looks like you only switched out an async for a callback.

davidwitten · 2020-08-04T20:08:15Z

@dyladan I can change it back, I just got a message in the Node8 test that I should add a done statement. It seems like it's fine now, but before it was timing out in one test in sdk-node

dyladan · 2020-08-04T20:13:51Z

The issue is that the test timed out. Adding a "done" statement is a suggestion of something you may have forgotten. In this case, it doesn't make the test less likely to time out.

dyladan · 2020-08-04T20:14:25Z

You're correct the test is flaky, but it is outside the scope of this PR. Please remove that change and I will merge this. I'm looking into the flaky test in another PR.

davidwitten · 2020-08-04T20:20:39Z

@dyladan Ok sounds good thanks! Just reverted the change

… Summary (open-telemetry#1320) Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>

* feat: graceful shutdown for tracing and metrics * fix: wording in test case * fix: typo * fix meterprovider config to use bracket notation Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com> * fix meterprovider config to use bracket notation Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com> * fix: add callbacks to shutdown methods * fix: merge conflict * simplify meter shutdown code Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com> * fix: fix one-liner * private function name style fix Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com> * fix: naming of private member variables * fix: graceful shutdown now works in browser * fix: window event listener will trigger once * fix: modify global shutdown helper functions * fix: remove callback from remove listener args * fix: change global shutdown function names and simplify functionality * fix: add rest of function refactoring and simplification * fix: remove unintended code snippet * fix: refactor naming of listener cleanup function and fix sandbox issue * fix: make global shutdown cleanup local * fix: change interval of MeterProvider collection to ensure it does not trigger through clock * chore: removing _cleanupGlobalShutdownListeners * fix: remove unnecesary trace provider member function * Removing default span attributes (#1342) * refactor(opentelemetry-tracing): removing default span attributes Signed-off-by: Aravin Sivakumar <aravinarjunn@gmail.com> * refactor(opentelemetry-tracing): removing default span attributed from tracer object Signed-off-by: Aravin Sivakumar <aravinarjunn@gmail.com> * refactor(opentelemetry-tracing): removing accidental add to package.json Signed-off-by: Aravin Sivakumar <aravinarjunn@gmail.com> * refactor(opentelemetry-tracing): removing redundant test and fixing suggestions by Shawn and Daniel Signed-off-by: Aravin Sivakumar <aravinarjunn@gmail.com> * feat: add baggage support to the opentracing shim (#918) Co-authored-by: Mayur Kale <mayurkale@google.com> * Add nodejs sdk package (#1187) Co-authored-by: Naseem <naseemkullah@gmail.com> Co-authored-by: legendecas <legendecas@gmail.com> Co-authored-by: Mark Wolff <mrw00010@gmail.com> Co-authored-by: Matthew Wear <matthew.wear@gmail.com> * feat: add OTEL_LOG_LEVEL env var (#974) * Proto update to latest to support arrays and maps (#1339) * chore: 0.10.0 release proposal (#1345) * fix: add missing grpc-js index (#1358) * chore: 0.10.1 release proposal (#1359) * feat(api/context-base): change compile target to es5 (#1368) * Feat: Make ID generator configurable (#1331) Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com> * fix: require grpc-js instead of grpc in grpc-js example (#1364) Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com> * chore(deps): update all non-major dependencies (#1371) * chore: bump metapackage dependencies (#1383) * chore: 0.10.2 proposal (#1382) * fix: remove unnecesary trace provider member function * refactor(metrics): distinguish different aggregator types (#1325) Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com> * Propagate b3 parentspanid and debug flag (#1346) * feat: Export MinMaxLastSumCountAggregator metrics to the collector as Summary (#1320) Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com> * feat: Collector Metric Exporter for the Web (#1308) Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com> * Fix issues in TypeScript getting started example code (#1374) Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com> * chore: deploy canary releases (#1384) * fix: protos pull * fix: address marius' feedback * chore: deleting removeAllListeners from prometheus, fixing tests, cleanu of events when using shutdown notifier * fix: add documentation and cleanup code * fix: remove async label from shutdown and cleanup test case * fix: update controller collect to return promise * fix: make downsides of disabling graceful shutdown more apparent Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com> Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com> Co-authored-by: Aravin <34178459+aravinsiva@users.noreply.github.com> Co-authored-by: Ruben Vargas Palma <ruben.vp8510@gmail.com> Co-authored-by: Mayur Kale <mayurkale@google.com> Co-authored-by: Naseem <naseemkullah@gmail.com> Co-authored-by: legendecas <legendecas@gmail.com> Co-authored-by: Mark Wolff <mrw00010@gmail.com> Co-authored-by: Matthew Wear <matthew.wear@gmail.com> Co-authored-by: Naseem <naseem@transit.app> Co-authored-by: Mark Wolff <mark.wolff@microsoft.com> Co-authored-by: Cong Zou <32532612+EdZou@users.noreply.github.com> Co-authored-by: Reginald McDonald <40721169+reggiemcdonald@users.noreply.github.com> Co-authored-by: WhiteSource Renovate <bot@renovateapp.com> Co-authored-by: srjames90 <srjames@lightstep.com> Co-authored-by: David W <dwitt12345@gmail.com> Co-authored-by: Mick Dekkers <mickdekkersnl@gmail.com>

…en-telemetry#1320) * fix(instrumentation-amqplib): move `@types/amqplib` into dev deps * refactor(instrumentation-amqplib): vendor in types into internal-types * refactor(instrumentation-amqplib): revert prettier formatting * refactor(instrumentation-amqplib): add code comments for vendoring * refactor(instrumentation-amqplib): delete unused file + run lint fixer * refactor(instrumentation-amqplib): fix ts eslint errorsOC * refactor(instrumentation-amqplib): fix lint --------- Co-authored-by: Amir Blum <amirgiraffe@gmail.com>

Changed metrics to functions and added summary

629fa50

davidwitten requested review from OlivierAlbertini, dyladan, legendecas, markwolff, mayurkale22, mwear, naseemkullah, obecny and vmarchaud as code owners July 16, 2020 15:35

Fixed tests

dc1a492

Merge remote-tracking branch 'upstream/master' into summary

5e040b1

dyladan approved these changes Jul 17, 2020

View reviewed changes

packages/opentelemetry-exporter-collector/src/transformMetrics.ts Outdated Show resolved Hide resolved

Remove extra assignment

52f1dec

obecny approved these changes Jul 20, 2020

View reviewed changes

packages/opentelemetry-exporter-collector/src/transformMetrics.ts Outdated Show resolved Hide resolved

packages/opentelemetry-exporter-collector/test/helper.ts Outdated Show resolved Hide resolved

packages/opentelemetry-exporter-collector/test/helper.ts Outdated Show resolved Hide resolved

davidwitten added 3 commits July 20, 2020 15:21

Merge remote-tracking branch 'upstream/master' into summary

e5fdf90

Used existing enum

f8a9abf

Added variable

dda64b3

dyladan reviewed Jul 20, 2020

View reviewed changes

packages/opentelemetry-exporter-collector/src/transformMetrics.ts Outdated Show resolved Hide resolved

davidwitten added 3 commits July 21, 2020 18:13

Merged

1a10562

One variable

360e5cb

Merge remote-tracking branch 'upstream/master' into summary

b60b053

Merge branch 'master' into summary

06719d8

mayurkale22 approved these changes Aug 4, 2020

View reviewed changes

dyladan added the enhancement New feature or request label Aug 4, 2020

dyladan changed the title ~~feat: Added Summary Data Points to the Collector~~ feat: Export Summary metrics to the collector Aug 4, 2020

dyladan changed the title ~~feat: Export Summary metrics to the collector~~ feat: Export MinMaxLastSumCountAggregator metrics to the collector as Summary Aug 4, 2020

Merge branch 'master' into summary

e32a05f

fix: flaky test

99415a3

vmarchaud approved these changes Aug 4, 2020

View reviewed changes

fix: reverted test changes

b8eda86

dyladan merged commit 1be50d7 into open-telemetry:master Aug 4, 2020

davidwitten mentioned this pull request Aug 6, 2020

Add Summary Data Point Support to the Exporter Collector #1294

Closed

dyladan linked an issue Aug 6, 2020 that may be closed by this pull request

Add Summary Data Point Support to the Exporter Collector #1294

Closed

davidwitten deleted the summary branch August 6, 2020 20:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Export MinMaxLastSumCountAggregator metrics to the collector as Summary#1320

feat: Export MinMaxLastSumCountAggregator metrics to the collector as Summary#1320
dyladan merged 14 commits intoopen-telemetry:masterfrom
davidwitten:summary

davidwitten commented Jul 16, 2020

Uh oh!

codecov bot commented Jul 16, 2020 •

edited

Loading

Uh oh!

dyladan left a comment

Uh oh!

Uh oh!

obecny left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dyladan commented Aug 4, 2020

Uh oh!

davidwitten commented Aug 4, 2020

Uh oh!

dyladan commented Aug 4, 2020

Uh oh!

davidwitten commented Aug 4, 2020 •

edited

Loading

Uh oh!

dyladan commented Aug 4, 2020

Uh oh!

dyladan commented Aug 4, 2020

Uh oh!

davidwitten commented Aug 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

davidwitten commented Jul 16, 2020

Uh oh!

codecov bot commented Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dyladan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

obecny left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dyladan commented Aug 4, 2020

Uh oh!

davidwitten commented Aug 4, 2020

Uh oh!

dyladan commented Aug 4, 2020

Uh oh!

davidwitten commented Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dyladan commented Aug 4, 2020

Uh oh!

dyladan commented Aug 4, 2020

Uh oh!

davidwitten commented Aug 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Jul 16, 2020 •

edited

Loading

davidwitten commented Aug 4, 2020 •

edited

Loading