[EXPORTER] Prometheus: Add unit to names, convert to word #2213

esigo · 2023-07-01T10:30:15Z

Fixes #1840 Fixes #2317 (issue)

Changes

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

CHANGELOG.md updated for non-trivial changes
Unit tests have been added
Changes in public API reviewed

codecov · 2023-07-01T10:46:33Z

Codecov Report

Merging #2213 (c80ebb9) into main (9b89843) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2213   +/-   ##
=======================================
  Coverage   87.43%   87.43%           
=======================================
  Files         199      199           
  Lines        6052     6052           
=======================================
  Hits         5291     5291           
  Misses        761      761

marcalff

I don't understand the inner loops of
PrometheusExporterUtils::TranslateToPrometheus(),
and the change there, please clarify.

It seems the name mapping is done for every data point of a metric, instead of once per metric.

marcalff · 2023-07-10T08:19:37Z

exporters/prometheus/src/exporter_utils.cc

+  }
+
+  std::vector<std::string> rate_entities;
+  size_t pos = rate_expressed_unit.find("/");


nit

do not execute the find twice, save the result in pos and test for pos == std::string::npos at the start of this method.

marcalff · 2023-07-10T08:34:00Z

exporters/prometheus/src/exporter_utils.cc

+        prometheus_client::MetricFamily metric_family;
+        metric_family.type = type;
+        metric_family.name = MapToPrometheusName(metric_data.instrument_descriptor.name_,
+                                                 metric_data.instrument_descriptor.unit_, type);
+        metric_family.help = metric_data.instrument_descriptor.description_;


This code moved inside the loop on data points.

The metric name, type, help, etc should be constant for all data points, so can this be moved outside ?

Even if type is needed and not available elsewhere, peek at the first data point before entering the loop on data points ?

+1. As long as it is safe to assume all data points have the same type, peeking at the first seems like the right thing to do. I took that approach in https://github.com/open-telemetry/opentelemetry-cpp/pull/2288/files#diff-e92aebc994eed5c134b09ecde972d9db39a9bfc46edc6d93b0b5a4c98e4ff27fR46.

thanks, moved back to where it was.

dashpole

Apologies for duplicating work, and thanks for working on this! It looks like this fixes #2287 as well, so i'll close my PR.

dashpole · 2023-09-01T18:32:50Z

exporters/prometheus/src/exporter_utils.cc

+        prometheus_client::MetricFamily metric_family;
+        metric_family.type = type;
+        metric_family.name = MapToPrometheusName(metric_data.instrument_descriptor.name_,
+                                                 metric_data.instrument_descriptor.unit_, type);
+        metric_family.help = metric_data.instrument_descriptor.description_;


+1. As long as it is safe to assume all data points have the same type, peeking at the first seems like the right thing to do. I took that approach in https://github.com/open-telemetry/opentelemetry-cpp/pull/2288/files#diff-e92aebc994eed5c134b09ecde972d9db39a9bfc46edc6d93b0b5a4c98e4ff27fR46.

dashpole · 2023-09-01T18:34:00Z

exporters/prometheus/src/exporter_utils.cc

@@ -114,8 +114,8 @@ std::vector<prometheus_client::MetricFamily> PrometheusExporterUtils::TranslateT
                "invalid SumPointData type");
          }
        }
+        output.emplace_back(metric_family);


Does the prometheus C++ library merge metric families? We definitely don't want one metric family for each data point.

thanks, creating new family only if it doesn't exist already.

dashpole · 2023-09-01T18:36:38Z

exporters/prometheus/src/exporter_utils.cc

+                                                  {"B", "bytes"},
+                                                  {"KB", "kilobytes"},
+                                                  {"MB", "megabytes"},
+                                                  {"GB", "gigabytes"},
+                                                  {"TB", "terabytes"},


We've removed B and $ from mappings elsewhere, as B means bel in UCUM. See open-telemetry/opentelemetry-java#5719

thanks, removed.

dashpole · 2023-09-01T18:42:11Z

exporters/prometheus/src/exporter_utils.cc

+  }
+
+  // Special case - counter
+  if (prometheus_type == prometheus_client::MetricType::Counter &&


Counters MUST end in _total, so we need to be less tolerant here. We can only skip the _total suffix if the name ends in _total. Since we need unit suffixes to come before type suffixes, one approach would be to trim _total suffixes from the name before appending the unit suffix, and finally append the final _total suffix.

thanks, will not add total unless if it has already.

exporters/prometheus/src/exporter_utils.cc

dashpole · 2023-09-05T14:30:08Z

exporters/prometheus/src/exporter_utils.cc

+  if (prometheus_type == prometheus_client::MetricType::Counter)
+  {
+    auto t_pos           = sanitized_name.rfind("_total");
+    bool ends_with_total = t_pos == sanitized_name.size() - 6;


Will this work if the metric has a unit? E.g. if I have a metric: foo.bar.total, with unit s, will I get foo_bar_total_seconds_total or foo_bar_seconds_total?

@dashpole the output will be foo_bar_total_seconds_total.

Ideally, you would trim _total from counters before appending the unit, and then always add _total after the unit for counters.

punya · 2023-09-23T09:32:14Z

I believe this would also fix #2317.

marcalff · 2023-10-19T12:34:24Z

Hi @esigo

Please merge with a recent main to resolve conflicts, so this PR can be reviewed and merged.

Thanks.

esigo · 2023-10-19T18:16:45Z

Hi @esigo

Please merge with a recent main to resolve conflicts, so this PR can be reviewed and merged.

Thanks.

Hi @marcalff thanks for the reminder. I finish this over the weekend.

owent · 2023-10-27T09:09:37Z

exporters/prometheus/src/collector.cc

@@ -39,7 +39,7 @@ std::vector<prometheus_client::MetricFamily> PrometheusCollector::Collect() cons
  reader_->Collect([&result](sdk::metrics::ResourceMetrics &metric_data) {
    auto prometheus_metric_data = PrometheusExporterUtils::TranslateToPrometheus(metric_data);
    for (auto &data : prometheus_metric_data)
-      result.emplace_back(data);
+      result.emplace_back(data.second);


Is it better to use std::move(data.second) here? ::prometheus::MetricFamily may contains a lot of data, and copy these data may have performance problem.

owent · 2023-10-27T09:10:55Z

exporters/prometheus/src/exporter_utils.cc

 {

  // initialize output vector
-  std::vector<prometheus_client::MetricFamily> output;
+  std::unordered_map<std::string, prometheus_client::MetricFamily> output;


Can we use output.reserve() first to reduce rehash?

dashpole · 2023-11-14T19:28:07Z

@esigo Is there any way I can help with this? I would love to get this over the finish line soon.

esigo · 2023-11-15T20:08:51Z

@esigo Is there any way I can help with this? I would love to get this over the finish line soon.

@dashpole thanks for the reminder. Becasue of the conflict I need to redo the PR. I finish this over the weekend.

…t-name

dashpole

Thanks!

exporters/prometheus/include/opentelemetry/exporters/prometheus/exporter_utils.h

lalitb

Thanks for the fix.

…s/exporter_utils.h Co-authored-by: David Ashpole <[email protected]>

This is mainly to get the fix made in open-telemetry/opentelemetry-cpp#2213 When opentelemetry-cpp makes a stable release with this fix, we'll switch to that. Closes #35151 COPYBARA_INTEGRATE_REVIEW=#35151 from yashykt:UpdateOTel 1041fbc PiperOrigin-RevId: 586815878

This is mainly to get the fix made in open-telemetry/opentelemetry-cpp#2213 When opentelemetry-cpp makes a stable release with this fix, we'll switch to that. Closes grpc#35151 COPYBARA_INTEGRATE_REVIEW=grpc#35151 from yashykt:UpdateOTel 1041fbc PiperOrigin-RevId: 586815878

…en-telemetry#2213)" This reverts commit 71d0e50

esigo added the area:exporter:prometheus Prometheus Exporter label Jul 1, 2023

Prometheus: Add unit to names, convert to word

06b2288

esigo force-pushed the 1840-Prometheus-unit-name branch from 940fed8 to 06b2288 Compare July 2, 2023 09:02

Merge branch 'main' into 1840-Prometheus-unit-name

fb15283

esigo marked this pull request as ready for review July 8, 2023 09:11

esigo requested a review from a team July 8, 2023 09:11

esigo changed the title ~~[WIP] Prometheus: Add unit to names, convert to word~~ Prometheus: Add unit to names, convert to word Jul 8, 2023

Merge branch 'main' into 1840-Prometheus-unit-name

41dda7b

marcalff reviewed Jul 10, 2023

View reviewed changes

Merge branch 'main' into 1840-Prometheus-unit-name

8da0585

marcalff mentioned this pull request Sep 1, 2023

Prometheus exporter does not add _total suffixes to counters #2287

Closed

dashpole reviewed Sep 1, 2023

View reviewed changes

dashpole mentioned this pull request Sep 1, 2023

Add _total suffix to prometheus counters #2288

Closed

esigo and others added 3 commits September 2, 2023 14:18

Merge branch 'main' into 1840-Prometheus-unit-name

c0dea97

review comments

ac9a1aa

c++11

1102d09

owent reviewed Sep 3, 2023

View reviewed changes

exporters/prometheus/src/exporter_utils.cc Outdated Show resolved Hide resolved

esigo and others added 2 commits September 3, 2023 12:13

review comments

8fb3e8a

Merge branch 'main' into 1840-Prometheus-unit-name

f34923c

dashpole reviewed Sep 5, 2023

View reviewed changes

punya mentioned this pull request Sep 23, 2023

[exporters/prometheus] Omit underscore suffix for unitless metrics #2323

Closed

3 tasks

review comments

638c5c9

owent reviewed Oct 27, 2023

View reviewed changes

esigo and others added 5 commits November 19, 2023 14:53

switch to vector

54a5686

Merge remote-tracking branch 'upstream/main' into 1840-Prometheus-uni…

cd310b9

…t-name

fix CI

71638d4

fix CI

53c1fa3

fix time

498caad

dashpole approved these changes Nov 21, 2023

View reviewed changes

exporters/prometheus/include/opentelemetry/exporters/prometheus/exporter_utils.h Outdated Show resolved Hide resolved

lalitb approved these changes Nov 21, 2023

View reviewed changes

lalitb and others added 2 commits November 21, 2023 09:52

Merge branch 'main' into 1840-Prometheus-unit-name

cc949cc

Update exporters/prometheus/include/opentelemetry/exporters/prometheu…

c80ebb9

…s/exporter_utils.h Co-authored-by: David Ashpole <[email protected]>

marcalff approved these changes Nov 22, 2023

View reviewed changes

marcalff changed the title ~~Prometheus: Add unit to names, convert to word~~ [EXPORTER] Prometheus: Add unit to names, convert to word Nov 22, 2023

marcalff merged commit 71d0e50 into open-telemetry:main Nov 22, 2023
45 checks passed

yashykt mentioned this pull request Nov 29, 2023

[otel] Update OTel to head grpc/grpc#35151

Closed

Porkepix mentioned this pull request Dec 7, 2023

opentelemetry-cpp 1.13.0 Homebrew/homebrew-core#156653

Merged

Yxang added a commit to Yxang/opentelemetry-cpp that referenced this pull request Sep 27, 2024

Revert "[EXPORTER] Prometheus: Add unit to names, convert to word (op…

282e97d

…en-telemetry#2213)" This reverts commit 71d0e50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EXPORTER] Prometheus: Add unit to names, convert to word #2213

[EXPORTER] Prometheus: Add unit to names, convert to word #2213

esigo commented Jul 1, 2023 •

edited

Loading

codecov bot commented Jul 1, 2023 •

edited

Loading

marcalff left a comment

marcalff Jul 10, 2023

marcalff Jul 10, 2023

dashpole Sep 1, 2023

esigo Sep 2, 2023

dashpole left a comment

dashpole Sep 1, 2023

dashpole Sep 1, 2023

esigo Sep 2, 2023 •

edited

Loading

dashpole Sep 1, 2023

esigo Sep 2, 2023

dashpole Sep 1, 2023

esigo Sep 2, 2023

dashpole Sep 5, 2023

esigo Oct 22, 2023

dashpole Oct 23, 2023

punya commented Sep 23, 2023

marcalff commented Oct 19, 2023

esigo commented Oct 19, 2023

owent Oct 27, 2023

owent Oct 27, 2023

dashpole commented Nov 14, 2023

esigo commented Nov 15, 2023

dashpole left a comment

lalitb left a comment

[EXPORTER] Prometheus: Add unit to names, convert to word #2213

[EXPORTER] Prometheus: Add unit to names, convert to word #2213

Conversation

esigo commented Jul 1, 2023 • edited Loading

Changes

codecov bot commented Jul 1, 2023 • edited Loading

Codecov Report

marcalff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dashpole left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esigo Sep 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

punya commented Sep 23, 2023

marcalff commented Oct 19, 2023

esigo commented Oct 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dashpole commented Nov 14, 2023

esigo commented Nov 15, 2023

dashpole left a comment

Choose a reason for hiding this comment

lalitb left a comment

Choose a reason for hiding this comment

esigo commented Jul 1, 2023 •

edited

Loading

codecov bot commented Jul 1, 2023 •

edited

Loading

esigo Sep 2, 2023 •

edited

Loading