example: add temporality selector option to metrics-basic by pitoniak32 · Pull Request #2217 · open-telemetry/opentelemetry-rust

pitoniak32 · 2024-10-16T12:25:23Z

Follow up PR from this comment: #2204 (review)

Changes

Add Temporality selector option to metrics-basic example

Merge requirement checklist

CONTRIBUTING guidelines followed
Unit tests added/updated (if applicable)
Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
Changes in public API reviewed (if applicable)

codecov · 2024-10-16T12:28:45Z

Codecov Report

Attention: Patch coverage is 43.28358% with 38 lines in your changes missing coverage. Please review.

Project coverage is 79.3%. Comparing base (f677458) to head (0c9e5c6).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
opentelemetry-otlp/src/metric.rs	0.0%	15 Missing ⚠️
opentelemetry-sdk/src/metrics/manual_reader.rs	0.0%	8 Missing ⚠️
opentelemetry-stdout/src/metrics/exporter.rs	0.0%	6 Missing ⚠️
opentelemetry-otlp/src/exporter/http/mod.rs	0.0%	3 Missing ⚠️
opentelemetry-sdk/src/metrics/instrument.rs	70.0%	3 Missing ⚠️
opentelemetry-otlp/src/exporter/tonic/mod.rs	0.0%	2 Missing ⚠️
opentelemetry-sdk/src/metrics/mod.rs	90.9%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #2217     +/-   ##
=======================================
+ Coverage   79.2%   79.3%   +0.1%     
=======================================
  Files        122     121      -1     
  Lines      20986   20944     -42     
=======================================
- Hits       16623   16612     -11     
+ Misses      4363    4332     -31

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Used the full path so it can just be uncommented without needing to adjust imports

utpilla · 2024-10-18T07:00:51Z

        assert_eq!(sum.data_points.len(), 1, "Expected only one data point");
        assert!(!sum.is_monotonic, "Should not produce monotonic.");
-        assert_eq!(sum.temporality, Temporality::Delta, "Should produce Delta");
+        assert_eq!(


Rename this test method to no_attr_up_down_counter now that its behavior has changed.

I updated to no_attr_up_down_counter_always_cumulative, and updated the assertion reason to indicate why Cumulative would be selected when Delta is provided for clarity in the future.

utpilla · 2024-10-18T07:12:26Z

 //!         .with_period(Duration::from_secs(3))
 //!         .with_timeout(Duration::from_secs(10))
-//!         .with_temporality_selector(DefaultTemporalitySelector::new())
+//!         .with_temporality(Temporality::default())


Following the same thing as #2217 (comment), we could just remove this.

utpilla

Looks good overall!

The public APIs exposed on MetricReader and PushMetricExporter traits might be updated in the future. That's mainly because it could be beneficial to push the "temporality + instrument kind" configuration solely to the exporter. That discussion and decision can happen independent of this PR.

pitoniak32 · 2024-10-18T11:40:06Z

Looks good overall!

The public APIs exposed on MetricReader and PushMetricExporter traits might be updated in the future. That's mainly because it could be beneficial to push the "temporality + instrument kind" configuration solely to the exporter. That discussion and decision can happen independent of this PR.

Awesome! I would be happy to take part in the discussion and decision around this topic when it comes up, I have been enjoying contributing to this code base and would like to take part more often :)

I will also update the CHANGELOG now that the larger concerns were addressed

cijothomas · 2024-10-18T17:49:17Z

Looks good overall!
The public APIs exposed on MetricReader and PushMetricExporter traits might be updated in the future. That's mainly because it could be beneficial to push the "temporality + instrument kind" configuration solely to the exporter. That discussion and decision can happen independent of this PR.

Awesome! I would be happy to take part in the discussion and decision around this topic when it comes up, I have been enjoying contributing to this code base and would like to take part more often :)

I will also update the CHANGELOG now that the larger concerns were addressed

Thanks!
some rough ideas:

Make temporality selection purely a configuration on MetricReader, and keep exporters away from it.
Exporters may offer methods that take MeterProviderBuilder as input, and returns the MeterProviderBuilder as output to make it easy for end users to enable exporters of their choice without having to worry about temporality.
eg:
meter_provider::builder()
.configure_resource(..)
.addFooExporter();

AddFooExporter() is offered by vendors/exporter authors which will add the right metric_reader with correct temporality etc automatically.

Of course this is not a requirement, so users can add reader themselves.

Alternatively, these traits can be provided with default implementation, so exporters need to override only if they wish to.

Closely related to that is the below issue. I am not a fan of OTLPExporter exposing duplicate concepts as that of SDK, so this could use some cleanups.
#1810

@pitoniak32 If you are up for it, please take a look at the above! Very happy to see more contributions from you.

pitoniak32 · 2024-10-18T19:35:27Z

@pitoniak32 If you are up for it, please take a look at the above! Very happy to see more contributions from you.

For sure! I will take a look at this one after work today

…etry#2217) Co-authored-by: Cijo Thomas <[email protected]>

pitoniak32 requested a review from a team as a code owner October 16, 2024 12:25

cijothomas reviewed Oct 16, 2024

View reviewed changes

Comment thread examples/metrics-basic/src/main.rs Outdated

utpilla reviewed Oct 17, 2024

View reviewed changes

Comment thread examples/metrics-basic/src/main.rs Outdated

pitoniak32 force-pushed the example/metrics-basic-temporality branch 3 times, most recently from 23494fc to 773803c Compare October 17, 2024 22:15

example: add temporality selector option to metrics-basic

472c62c

Used the full path so it can just be uncommented without needing to adjust imports

pitoniak32 requested review from cijothomas and utpilla October 17, 2024 22:17

pitoniak32 force-pushed the example/metrics-basic-temporality branch from 4561323 to 2fcb26a Compare October 17, 2024 22:50

utpilla reviewed Oct 17, 2024

View reviewed changes

Comment thread examples/metrics-basic/src/main.rs Outdated

utpilla reviewed Oct 17, 2024

View reviewed changes

Comment thread opentelemetry-sdk/src/metrics/instrument.rs Outdated

pitoniak32 force-pushed the example/metrics-basic-temporality branch from c8e1fa1 to 2e6a0af Compare October 17, 2024 23:42

pitoniak32 added 5 commits October 17, 2024 19:43

refactor: use temporality enum for selection

12e3af5

docs: fix broken links

530c8e9

test: fix UpDownCounter logic to account for temporality preference

cadfda2

fix: scope temporality_preference function to crate

df0c0aa

example: remove explicit call to Temporality default

a7210f6

pitoniak32 force-pushed the example/metrics-basic-temporality branch from 2e6a0af to a7210f6 Compare October 17, 2024 23:45

utpilla reviewed Oct 18, 2024

View reviewed changes

utpilla approved these changes Oct 18, 2024

View reviewed changes

pitoniak32 and others added 4 commits October 18, 2024 08:45

test: update name for up_down_counter instrument

5906d94

doc: remove explicit default call for configuring temporality

1fe18fd

chore: add CHANGELOG entries for each crate for API changes

1458af3

Merge branch 'main' into example/metrics-basic-temporality

0c9e5c6

cijothomas merged commit 4852a5e into open-telemetry:main Oct 18, 2024

pitoniak32 deleted the example/metrics-basic-temporality branch October 18, 2024 19:34

cijothomas mentioned this pull request Nov 7, 2024

Cleanup Temporality Selector #2099

Closed

pitoniak32 mentioned this pull request Nov 22, 2024

REQUEST: New membership for pitoniak32 open-telemetry/community#2452

Closed

6 tasks

takumi-earth pushed a commit to earthlings-dev/opentelemetry-rust that referenced this pull request Jan 27, 2026

example: add temporality selector option to metrics-basic (open-telem…

f644828

…etry#2217) Co-authored-by: Cijo Thomas <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

example: add temporality selector option to metrics-basic#2217

example: add temporality selector option to metrics-basic#2217
cijothomas merged 10 commits into
open-telemetry:mainfrom
pitoniak32:example/metrics-basic-temporality

pitoniak32 commented Oct 16, 2024 •

edited

Loading

Uh oh!

codecov Bot commented Oct 16, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

utpilla Oct 18, 2024

Uh oh!

pitoniak32 Oct 18, 2024

Uh oh!

utpilla Oct 18, 2024

Uh oh!

utpilla left a comment

Uh oh!

pitoniak32 commented Oct 18, 2024

Uh oh!

cijothomas commented Oct 18, 2024

Uh oh!

pitoniak32 commented Oct 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

pitoniak32 commented Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Merge requirement checklist

Uh oh!

codecov Bot commented Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

utpilla Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

pitoniak32 Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

utpilla Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

utpilla left a comment

Choose a reason for hiding this comment

Uh oh!

pitoniak32 commented Oct 18, 2024

Uh oh!

cijothomas commented Oct 18, 2024

Uh oh!

pitoniak32 commented Oct 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pitoniak32 commented Oct 16, 2024 •

edited

Loading

codecov Bot commented Oct 16, 2024 •

edited

Loading