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.

Sign up for GitHub

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

Jump to bottom

feat(metric-reader): add metric-reader #2681

Merged
merged 22 commits into from
Jan 7, 2022
Merged

feat(metric-reader): add metric-reader #2681

merged 22 commits into from
Jan 7, 2022

Conversation

pichlermarc
Copy link
Member

@pichlermarc pichlermarc commented Dec 21, 2021 •
edited
Loading

Which problem is this PR solving?

This is the implementation of the MetricReader and PeriodicExportingMetricReader described in the the OTel JavaScript Metrics SDK Design Doc and the Metrics SDK Spec

Original issue text:

The MetricReader handles collecting metrics from all SDK instruments. It also handles shutdown and forceflush signals from the SDK.

MetricReader

  • collect
  • shutdown
  • forceFlush

Periodic exporting metric reader

  • Collects metrics on a time interval and passes them to a push metric exporter
  • Configurations
    • exporter
    • export interval (ms)
    • export timeout (ms)

Closes #2590

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

  • Unit tests in opentelemetry-sdk-metric-base/test/export

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@codecov
Copy link

codecov bot commented Dec 21, 2021 •
edited
Loading

Codecov Report

Merging #2681 (056ea19) into main (d61f7be) will decrease coverage by 0.10%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2681      +/-   ##
==========================================
- Coverage   92.82%   92.72%   -0.11%     
==========================================
  Files         153      155       +2     
  Lines        5267     5372     +105     
  Branches     1112     1133      +21     
==========================================
+ Hits         4889     4981      +92     
- Misses        378      391      +13
Impacted Files Coverage Δ
...pentelemetry-sdk-metrics-base/src/MeterProvider.ts 17.50% <0.00%> (ø)
...s-base/src/export/PeriodicExportingMetricReader.ts 100.00% <0.00%> (ø)
...ckages/opentelemetry-sdk-metrics-base/src/utils.ts 96.00% <0.00%> (+6.00%) ⬆️
...etry-sdk-metrics-base/src/export/MetricExporter.ts 52.94% <0.00%> (+29.41%) ⬆️
...emetry-sdk-metrics-base/src/export/MetricReader.ts 97.29% <0.00%> (+73.48%) ⬆️

@pichlermarc pichlermarc marked this pull request as ready for review December 22, 2021 16:30
@pichlermarc pichlermarc requested a review from a team December 22, 2021 16:30
legendecas
legendecas reviewed Dec 22, 2021
View reviewed changes
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

👍 nice work!

experimental/packages/opentelemetry-sdk-metrics-base/src/export/MetricReader.ts Outdated Show resolved Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/src/export/MetricReader.ts Outdated Show resolved Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/src/export/MetricReader.ts Outdated Show resolved Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/src/export/MetricReader.ts Outdated Show resolved Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/src/export/MetricReader.ts Outdated Show resolved Hide resolved
...rimental/packages/opentelemetry-sdk-metrics-base/src/export/PeriodicExportingMetricReader.ts Outdated Show resolved Hide resolved
...rimental/packages/opentelemetry-sdk-metrics-base/src/export/PeriodicExportingMetricReader.ts Outdated Show resolved Hide resolved
@dyladan dyladan mentioned this pull request Dec 29, 2021
Closed
legendecas
legendecas approved these changes Jan 6, 2022
View reviewed changes
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

LGTM % nits

experimental/packages/opentelemetry-sdk-metrics-base/src/export/MetricReader.ts Outdated Show resolved Hide resolved
...al/packages/opentelemetry-sdk-metrics-base/test/export/PeriodicExportingMetricReader.test.ts Outdated Show resolved Hide resolved
...al/packages/opentelemetry-sdk-metrics-base/test/export/PeriodicExportingMetricReader.test.ts Outdated Show resolved Hide resolved
...al/packages/opentelemetry-sdk-metrics-base/test/export/PeriodicExportingMetricReader.test.ts Outdated Show resolved Hide resolved
...al/packages/opentelemetry-sdk-metrics-base/test/export/PeriodicExportingMetricReader.test.ts Outdated Show resolved Hide resolved
jsuereth
jsuereth approved these changes Jan 6, 2022
View reviewed changes
experimental/packages/opentelemetry-sdk-metrics-base/src/export/MetricReader.ts
* Set the {@link MetricProducer} used by this instance.
*
* @param metricProducer
*/
setMetricProducer(metricProducer: MetricProducer) {
Copy link

Choose a reason for hiding this comment

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

Ah, is this taking the Java route of passing a metric-producer rather than hard-coding collect to talk to static registry of metrics.

I'm wondering if we should update the specification so that this is the "default" specified mechanism and the .NET hook is an alternative option to make it more clear to implementers that wish to a hard-link between metric-reader implementation + the sdk. Did you find this confusing at all?

Copy link
Member

Choose a reason for hiding this comment

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

It is confusing on the spec side. I've previously submitted issues like open-telemetry/opentelemetry-specification#2072 for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that it is somewhat confusing on the spec side, when I was reading it the same question described in the issue above popped into my mind. I think updating the spec would help in that regard.

@pichlermarc
feat(metric-reader): add metric-reader.
6c4f989
@pichlermarc
refactor(metric-reader): use callbacks
4a767a7
@pichlermarc
refactor(metric-reader): add options so that timeout can be omitted.
fd04ff4
@pichlermarc
refactor(metric-reader): combine TestMetricExporter with WaitingMetri…
df2af4a 
…cExporter
@pichlermarc
fix(metric-reader): update shutdown and forceFlush usages, fix style.
8390e4a
@pichlermarc
refactor(metric-reader): add default implementation for onInitialized…
78d582d 
…(), add TestMetricReader
@pichlermarc
refactor(metric-reader): use promise pattern instead of callbacks
381c8a0
@pichlermarc
refactor(metric-reader): update metric collector to use the promise p…
f9ec116 
…attern.
@pichlermarc
fix(metric-reader): pass function instead of Promise to assert.reject…
fcfe6b7 
…s to keep node 8 compatibility.
@pichlermarc
fix(metric-reader): do not collect and export before force-flushing t…
6d2690b 
…he exporter.
@pichlermarc
refactor(metric-reader): remove unused ReaderResult
2346295
@pichlermarc
fix(metric-reader): do not throw on multiple shutdown calls.
59697cf
@pichlermarc
refactor(metric-reader): move callWithTimeout and TimeoutError to src…
5fa2f39 
…/utils.ts
@pichlermarc
docs(metric-reader): add link to describe why the prototype is set ma…
5081c9c 
…nually in TimeoutError
@pichlermarc
fix(metric-reader): fix switched-out reader and force-flush options.
3158422
@pichlermarc
fix(metric-reader): do not use default timeouts.
282a75d
dyladan
dyladan approved these changes Jan 7, 2022
View reviewed changes
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

LGTM thanks for the great work

experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricCollector.ts Outdated Show resolved Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricCollector.ts Outdated Show resolved Hide resolved
@dyladan dyladan merged commit 354c002 into open-telemetry:main Jan 7, 2022
@dyladan dyladan deleted the metric-reader branch January 7, 2022 18:48
rauno56 pushed a commit to rauno56/opentelemetry-js that referenced this pull request Jan 14, 2022
@pichlermarc @rauno56
feat(metric-reader): add metric-reader (open-telemetry#2681)
5e327e5
@dyladan dyladan added the enhancement New feature or request label Jan 31, 2022
@legendecas legendecas mentioned this pull request Feb 15, 2022
Merged
@dyladan dyladan mentioned this pull request Mar 16, 2022
Merged
@pichlermarc pichlermarc mentioned this pull request Apr 22, 2022
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@legendecas legendecas legendecas approved these changes

@srikanthccv srikanthccv srikanthccv left review comments

@jsuereth jsuereth jsuereth approved these changes

@dyladan dyladan dyladan approved these changes

@naseemkullah naseemkullah naseemkullah approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement MetricReader
6 participants
@pichlermarc @jsuereth @dyladan @legendecas @srikanthccv @naseemkullah