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

Add OTLPMetricPipeline builder methods for delta and lowmemory #1057

Conversation

cijothomas
Copy link
Member

@cijothomas cijothomas commented May 9, 2023

Changes

Add new builder methods to help build OTLP Metrics Pipeline with desired Temporality. This PR specifically adds
builders to support the "Delta" and "LowMemory" temporality preference from the spec:
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk_exporters/otlp.md#additional-configuration

Note that the env variable support for the same (OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE), is not part of this PR, and will be offered as a separate PR.

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)

@cijothomas cijothomas requested a review from a team May 9, 2023 06:28

### Added
- Added builder methods to configure delta, low-memory temporality for OTLP
Metric pipeline
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Add PR #

pub(crate) _private: (),
}

impl DeltaTemporalitySelector {
Copy link
Contributor

Choose a reason for hiding this comment

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

We have the same thing defined in benches currently. Is it worthwhile to consolidate this into a common place?

Copy link
Contributor

@hdost hdost left a comment

Choose a reason for hiding this comment

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

Aside from the linting issues idea makes sense. Although I concur with @lzchen that maybe the selectors should be somewhere common, SDK perhaps. This would allow others to use them as well.

/// [Temporality::Cumulative] will be used for UpDownCounter and Asynchronous UpDownCounter
#[derive(Clone, Default, Debug)]
struct DeltaTemporalitySelector {
pub(crate) _private: (),
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the benefit of this?

Copy link
Member

@jtescher jtescher left a comment

Choose a reason for hiding this comment

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

I think this is basically good to merge, only a couple nits remaining.


impl LowMemoryTemporalitySelector {
/// Create a new default temporality selector.
fn new() -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

nit: as the lint is pointing out, are you intending this to be pub? or should this be removed as it's not used.

Copy link
Contributor

@hdost hdost left a comment

Choose a reason for hiding this comment

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

Need to rebase and fix lints.

@cijothomas
Copy link
Member Author

Closing as #1568 covered this.

@cijothomas cijothomas closed this Mar 13, 2024
@cijothomas cijothomas deleted the cijothomas/otlp-metric-temporality-helpers branch March 13, 2024 01:39
@pitoniak32 pitoniak32 mentioned this pull request Oct 12, 2024
4 tasks
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.

4 participants