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

prometheus: add without_counter_suffixes option #1158

Merged
merged 1 commit into from
Jul 19, 2023

Conversation

jtescher
Copy link
Member

Adds

This adds a method on the prometheus exporter config to not include counter suffixes as required in this spec change open-telemetry/opentelemetry-specification#3590

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)

@jtescher jtescher requested a review from a team July 19, 2023 01:26
@jtescher jtescher force-pushed the add-without-suffix-option branch from d60598e to cc8545d Compare July 19, 2023 01:27
@codecov
Copy link

codecov bot commented Jul 19, 2023

Codecov Report

Patch coverage: 87.6% and no project coverage change.

Comparison is base (2c35c55) 49.4% compared to head (cc8545d) 49.4%.

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #1158   +/-   ##
=====================================
  Coverage   49.4%   49.4%           
=====================================
  Files        164     164           
  Lines      20386   20416   +30     
=====================================
+ Hits       10072   10105   +33     
+ Misses     10314   10311    -3     
Impacted Files Coverage Δ
opentelemetry-prometheus/src/config.rs 76.0% <83.3%> (+0.6%) ⬆️
opentelemetry-prometheus/src/lib.rs 86.3% <88.0%> (+1.8%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

impl Collector {
fn metric_type_and_name(&self, m: &data::Metric) -> Option<(MetricType, Cow<'static, str>)> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this later when add_* functions? It may save us one downcast/runtime type check. I do see this version is easier to understand and less repetitive though but wonder if the performance wise if it make sense to avoid repeating downcast when the type is Sum

Copy link
Member Author

Choose a reason for hiding this comment

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

We could, but I don't think the downcasting has any noticeable perf impact. Could do a benchmark but threads like https://users.rust-lang.org/t/downcasting-overhead/69718/6 make it pretty clear.

Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't seem to make a difference when I run quick bench, can re-assess if perf becomes an issue later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sg. Thanks for checking!

@jtescher jtescher merged commit db1fedb into open-telemetry:main Jul 19, 2023
@jtescher jtescher deleted the add-without-suffix-option branch July 19, 2023 19:32
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