Skip to content

feat(sdk-metrics): implement metric reader metrics#6449

Open
anuraaga wants to merge 17 commits intoopen-telemetry:mainfrom
anuraaga:metrics-reader-metrics
Open

feat(sdk-metrics): implement metric reader metrics#6449
anuraaga wants to merge 17 commits intoopen-telemetry:mainfrom
anuraaga:metrics-reader-metrics

Conversation

@anuraaga
Copy link
Copy Markdown
Contributor

Which problem is this PR solving?

I am helping to implement SDK internal metrics

https://opentelemetry.io/docs/specs/semconv/otel/sdk-metrics/

This implements metric reader metrics.

Implementation in Java - https://github.com/open-telemetry/opentelemetry-java/pull/8038/changes

/cc @trentm

Short description of the changes

Implement metrics for metric collections per semconv

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Unit tests

Checklist:

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

@anuraaga anuraaga requested a review from a team as a code owner February 26, 2026 06:19
@anuraaga anuraaga force-pushed the metrics-reader-metrics branch from 4c1dd02 to 0e63817 Compare February 26, 2026 06:19
@anuraaga anuraaga force-pushed the metrics-reader-metrics branch from 0e63817 to 5beee50 Compare February 26, 2026 06:19
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 26, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.79%. Comparing base (8ee2a8b) to head (3f4c342).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6449      +/-   ##
==========================================
+ Coverage   95.76%   95.79%   +0.02%     
==========================================
  Files         375      378       +3     
  Lines       12739    12780      +41     
  Branches     3013     3020       +7     
==========================================
+ Hits        12200    12242      +42     
+ Misses        539      538       -1     
Files with missing lines Coverage Δ
...etry-exporter-prometheus/src/PrometheusExporter.ts 96.73% <100.00%> (+0.03%) ⬆️
...s/opentelemetry-exporter-prometheus/src/semconv.ts 100.00% <100.00%> (ø)
...imental/packages/opentelemetry-sdk-node/src/sdk.ts 95.93% <ø> (ø)
packages/opentelemetry-core/src/common/time.ts 97.10% <100.00%> (+0.08%) ⬆️
packages/sdk-metrics/src/MeterProvider.ts 100.00% <100.00%> (ø)
packages/sdk-metrics/src/export/MetricReader.ts 100.00% <100.00%> (ø)
...ages/sdk-metrics/src/export/MetricReaderMetrics.ts 100.00% <100.00%> (ø)
...etrics/src/export/PeriodicExportingMetricReader.ts 100.00% <100.00%> (+1.26%) ⬆️
packages/sdk-metrics/src/semconv.ts 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

}

const collectDuration = hrTimeDuration(startTime, endTime);
const collectDurationSecs = collectDuration[0] + collectDuration[1] / 1e9;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Let me know if I missed something, I was surprised to not find hrTimeToSeconds since there are many metrics defined as seconds

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it would be worthwhile adding hrTimeToSeconds to "packages/opentelemetry-core/src/common/time.ts". That could totally be in a separate PR, though. Or an issue would be welcome.

* - Use a domain-specific attribute
* - Set `error.type` to capture all errors, regardless of whether they are defined within the domain-specific set or not.
*/
export const ATTR_ERROR_TYPE = 'error.type' as const;
Copy link
Copy Markdown
Contributor Author

@anuraaga anuraaga Feb 26, 2026

Choose a reason for hiding this comment

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

This one is actually stable but wasn't sure it's worth adding a dep on semconv for it

Copy link
Copy Markdown
Contributor

@trentm trentm Feb 27, 2026

Choose a reason for hiding this comment

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

Understood. I don't have a strong opinion. If the semconv package was smaller we wouldn't hesistate to add the dep. :)

I imagine this usage will break attempts to use https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/scripts/gen-semconv-ts.js to re-generate this semconv.ts file, but that's not a real concern right now. That script is currently a best-effort helper and not mandated at all.

@anuraaga anuraaga changed the title feat(metrics): implement metric reader metrics feat(sdk-metrics): implement metric reader metrics Feb 26, 2026
Comment thread packages/sdk-metrics/src/export/MetricReaderMetrics.ts
* - Use a domain-specific attribute
* - Set `error.type` to capture all errors, regardless of whether they are defined within the domain-specific set or not.
*/
export const ATTR_ERROR_TYPE = 'error.type' as const;
Copy link
Copy Markdown
Contributor

@trentm trentm Feb 27, 2026

Choose a reason for hiding this comment

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

Understood. I don't have a strong opinion. If the semconv package was smaller we wouldn't hesistate to add the dep. :)

I imagine this usage will break attempts to use https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/scripts/gen-semconv-ts.js to re-generate this semconv.ts file, but that's not a real concern right now. That script is currently a best-effort helper and not mandated at all.

this._metrics = new MetricReaderMetrics(
OTEL_COMPONENT_TYPE_VALUE_PERIODIC_METRIC_READER,
meter
);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What about moving the MetricReaderMetrics instance to the MetricReader base class:

  • setMeterProvider moves to class MetricReader (and the method gets added to interface IMetricReader)
  • The setMeterProvider needs a value for otel.component.type, so a class extending MetricReader should be able to pass that in:
    • Add an optional otelComponentType string option to interface MetricReaderOptions
    • Get class PeriodicExportingMetricReader to pass in OTEL_COMPONENT_TYPE_VALUE_PERIODIC_METRIC_READER in its call to super()
    • Get class MetricReader to save that to private readonly _otelComponentType: string | undefined;
    • Get the implementation of setMeterProvider to use this._otelComponentType || this.constructor.name. This will allow a reasonable default value for MetricReader subclasses that don't fit one of the well-known names for otel.component.type. (From the spec: "otel.component.type: If none of the standardized values apply, implementations SHOULD use the language-defined name of the type. E.g. for Java the fully qualified classname SHOULD be used in this case.")
  • Move the handling for this._metrics.recordCollection(...) to MetricReader#collect().
  • Then it would also be good to update "experimental/packages/opentelemetry-exporter-prometheus/src/PrometheusExporter.ts" to pass in otelComponentType: OTEL_COMPONENT_TYPE_VALUE_PROMETHEUS_HTTP_TEXT_METRIC_EXPORTER in its call to super()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good idea that makes sense - did it and also the prometheus change. One deviation from this comment I resolved _otelComponentType to the automatic value in the constructor instead of setMeterProvider, let me know if I'm missing some JS intricacy that means that shouldn't be done.

}

const collectDuration = hrTimeDuration(startTime, endTime);
const collectDurationSecs = collectDuration[0] + collectDuration[1] / 1e9;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it would be worthwhile adding hrTimeToSeconds to "packages/opentelemetry-core/src/common/time.ts". That could totally be in a separate PR, though. Or an issue would be welcome.

Comment thread CHANGELOG.md
Comment thread CHANGELOG.md Outdated
Copy link
Copy Markdown
Contributor

@trentm trentm left a comment

Choose a reason for hiding this comment

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

LGTM, modulo the changelog nit.

@pichlermarc Would you like to sanity check anything here? E.g. the addition of setMeterProvider to IMetricReader, perhaps. I'm not super-confident in the Metrics SDK.

Comment thread CHANGELOG.md Outdated
@anuraaga
Copy link
Copy Markdown
Contributor Author

anuraaga commented Apr 2, 2026

Thanks @trentm - updated

@pichlermarc
Copy link
Copy Markdown
Member

pichlermarc commented Apr 7, 2026

LGTM, modulo the changelog nit.

@pichlermarc Would you like to sanity check anything here? E.g. the addition of setMeterProvider to IMetricReader, perhaps. I'm not super-confident in the Metrics SDK.

I think the implementation looks solid - only issue I could see is that setMeterProvider() could prove confusing to users. For example somebody doing something like:

metrics.setGlobalMeterProvider(new MeterProvider());
const meterProvider = metrics.getMeterProvider();

const reader = new PeriodicExportingMetricReader({exporter: myExporter});
reader.setMeterProvider(meterProvider); // did not read the docs, why are no metrics exported?

I wonder if setSelfMonitoringMeterProvider() or another more descriptive name could help.

@anuraaga
Copy link
Copy Markdown
Contributor Author

anuraaga commented Apr 9, 2026

reader.setMeterProvider(meterProvider); // did not read the docs, why are no metrics exported?

That's a good point, thanks for bringing it up. In Java and Python, this method is actually private, either accessed by reflection or using Python's _ prefix naming scheme. Does it make sense to use a _ prefix here in JS too?

@anuraaga
Copy link
Copy Markdown
Contributor Author

anuraaga commented Apr 9, 2026

I think the use case for providing metrics for direct implementations of IMetricReader are low vs subclassing MetricReader. I reworked to remove from the interface and make the method _. Let me know if this seems reasonable, or I can revert it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants