Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(metrics): distinguish different aggregator types #1325

Merged
merged 4 commits into from
Aug 4, 2020

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Jul 17, 2020

Which problem is this PR solving?

Separate the toPoint definition to distinguish the return value of one aggregator possibly returning.

The change made to Aggregator is that one aggregator is determined to return one type of point value type.

Currently, the Aggregator definition returns any one of the point value types in a single aggregator, i.e. an aggregator can return a LastValue and a Histogram for each toPoint call since they all conform to the signature.

const metricRecord: MetricRecord = getRecordSomeWhere()

if (metricRecord.aggregator.kind === AggregatorKind.HISTOGRAM) {
  const point = metricRecord.aggregator.toPoint()
  // this point is infered as Point<PointValueType>, so type casting is still needed.
  // accessing `point.value.count` needs a type casting
}

The solution proposed in the PR made an aggregator can only return a definite subset of PointValueType. i.e. an aggregator with HistogramAggregatorType returning a LastValue is not going to pass the type check. This made the aggregator kind check prominent and enforced.

const metricRecord: MetricRecord = getRecordSomeWhere()

if (metricRecord.aggregator.kind === AggregatorKind.HISTOGRAM) {
  const point = metricRecord.aggregator.toPoint()
  // this point can be infered as Point<Histogram> without any manual type casting 
  // since aggregator can be determined as type of SumAggregatorType for
  //  `metricRecord.aggregator.kind === AggregatorKind.HISTOGRAM` branching.
}

Short description of the changes

Split Aggregator type definition to several concrete AggregatorTypes, like SumAggregatorType and HistogramAggregatorType.

Separate the `toPoint` definition to distinguish the return value of
one aggregator possibly returning.
@codecov
Copy link

codecov bot commented Jul 17, 2020

Codecov Report

Merging #1325 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1325      +/-   ##
==========================================
+ Coverage   93.61%   93.63%   +0.01%     
==========================================
  Files         149      149              
  Lines        4261     4272      +11     
  Branches      870      871       +1     
==========================================
+ Hits         3989     4000      +11     
  Misses        272      272              
Impacted Files Coverage Δ
...emetry-metrics/src/export/aggregators/Histogram.ts 100.00% <100.00%> (ø)
...trics/src/export/aggregators/MinMaxLastSumCount.ts 100.00% <100.00%> (ø)
...pentelemetry-metrics/src/export/aggregators/Sum.ts 100.00% <100.00%> (ø)
packages/opentelemetry-metrics/src/export/types.ts 100.00% <100.00%> (ø)

Copy link
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

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

👍

@dyladan dyladan added the enhancement New feature or request label Jul 22, 2020
# Conflicts:
#	packages/opentelemetry-metrics/src/export/aggregators/Histogram.ts
@legendecas
Copy link
Member Author

@open-telemetry/javascript-maintainers hey folks, this PR has been open for 17 days, and one more approval is required to merge this PR. However, there is a statement on the document that "A pull request opened by an approver may be merged with only 3 reviews.". If you think this PR looks good, please find a time to merge it :)

@dyladan dyladan merged commit 6eb51a2 into open-telemetry:master Aug 4, 2020
@legendecas legendecas deleted the aggregator-type branch August 5, 2020 06:11
jonahrosenblum pushed a commit to jonahrosenblum/opentelemetry-js that referenced this pull request Aug 8, 2020
…etry#1325)

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
obecny added a commit that referenced this pull request Aug 17, 2020
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants