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

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.

examples/metrics-basic/src/main.rs

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

examples/metrics-basic/src/main.rs

opentelemetry-sdk/src/metrics/instrument.rs

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

opentelemetry-sdk/src/metrics/mod.rs

@@ -945,7 +944,11 @@ mod tests {

 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

opentelemetry-otlp/src/lib.rs

@@ -183,7 +183,7 @@
 //! .with_resource(Resource::new(vec![KeyValue::new("service.name", "example")]))
 //! .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

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

cijothomas reviewed Oct 16, 2024

View reviewed changes

examples/metrics-basic/src/main.rs Outdated Show resolved Hide resolved

utpilla reviewed Oct 17, 2024

View reviewed changes

examples/metrics-basic/src/main.rs Outdated Show resolved Hide resolved

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

examples/metrics-basic/src/main.rs Outdated Show resolved Hide resolved

utpilla reviewed Oct 17, 2024

View reviewed changes

opentelemetry-sdk/src/metrics/instrument.rs Outdated Show resolved Hide resolved

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
24 of 25 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

pitoniak32 commented Oct 16, 2024 •

edited

Loading

codecov bot commented Oct 16, 2024 •

edited

Loading

utpilla Oct 18, 2024

pitoniak32 Oct 18, 2024

utpilla Oct 18, 2024

utpilla left a comment

pitoniak32 commented Oct 18, 2024

cijothomas commented Oct 18, 2024

pitoniak32 commented Oct 18, 2024

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

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

Conversation

pitoniak32 commented Oct 16, 2024 • edited Loading

Changes

Merge requirement checklist

codecov bot commented Oct 16, 2024 • edited Loading

Codecov Report

utpilla Oct 18, 2024

Choose a reason for hiding this comment

pitoniak32 Oct 18, 2024

Choose a reason for hiding this comment

utpilla Oct 18, 2024

Choose a reason for hiding this comment

utpilla left a comment

Choose a reason for hiding this comment

pitoniak32 commented Oct 18, 2024

cijothomas commented Oct 18, 2024

pitoniak32 commented Oct 18, 2024

pitoniak32 commented Oct 16, 2024 •

edited

Loading

codecov bot commented Oct 16, 2024 •

edited

Loading