Add Prometheus Exporter: Step 1 #280

CunjunWang · 2020-08-17T23:10:15Z

This PR implements exporter utils helper functions for the Prometheus Exporter project. The C++ file prometheus_exporter_utils.cc implements all the helper functions that will be called when we translate a OpenTelemetry (OTLP) metric to a Prometheus metric.

Changes

Modified the WORKSPACE file and added C++ Prometheus Client as a dependency to the project.
Modified CMakeLists.txt file in exporters folder to add Prometheus Exporter as a sub-project.
Added BUILD file in exporters/prometheus to support building project and the tests with Bazel.
Added CMakeLists.txt file in exporters/prometheus to support building project with CMake.
Added function header definition in exporters/prometheus/include
Implemented the header in exporters/prometheus/src/prometheus_exporter_utils.cc
Tested the implementation with unit tests in exporters/prometheus/test/prometheus_exporter_utils_test.cc
Added CMakeLists.txt file in exporters/prometheus/test to support building tests with CMake.
Modified ci/setup_cmake.sh to configure Prometheus C++ client as a dependency
Modified .github/workflows/ci.yml to execute Prometheus Exporter test sub-job
Modified ci/do_ci.sh to add Prometheus Exporter test setup scripts, and ignore Prometheus tests in bazel.noexcept

…and unit test file

CunjunWang · 2020-08-17T23:17:21Z

For the CI, we actually need to have prometheus-cpp library installed in the CI environment because it's a project dependency

codecov · 2020-08-18T03:49:21Z

Codecov Report

Merging #280 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #280   +/-   ##
=======================================
  Coverage   94.55%   94.55%           
=======================================
  Files         146      146           
  Lines        6610     6610           
=======================================
  Hits         6250     6250           
  Misses        360      360

… windows with vcpkg

reyang · 2020-08-21T02:31:49Z

For the CI, we actually need to have prometheus-cpp library installed in the CI environment because it's a project dependency

Could you add a GitHub Actions to cover that? You can potentially leverage a linux container and install the dependency you need for testing. Here goes and example of how to do that https://github.com/open-telemetry/opentelemetry-dotnet/tree/master/test/OpenTelemetry.Instrumentation.StackExchangeRedis.Tests.

…Wang/opentelemetry-cpp into prometheus-exporter-utils

g-easy · 2020-08-24T04:04:52Z

WORKSPACE

+
+http_archive(
+    name = "com_github_jupp0r_prometheus_cpp",
+    strip_prefix = "prometheus-cpp-master",


Please pin to a version (and add sha256), for consistency with other deps.

pin to v0.9.0, and sha256 added

g-easy · 2020-08-24T04:07:04Z

exporters/prometheus/include/opentelemetry/exporters/prometheus/prometheus_exporter_utils.h

+#include "opentelemetry/sdk/metrics/record.h"
+#include "prometheus/metric_family.h"
+
+namespace prometheus_client = ::prometheus;


Is there a way to limit the scope of these? I'm worried about having them in a header file.

removed it from header, and put it in implementation files

g-easy · 2020-08-24T04:08:14Z

exporters/prometheus/src/prometheus_exporter_utils.cc

+ * @return a collection of translated metrics that is acceptable by Prometheus
+ */
+std::vector<prometheus_client::MetricFamily> PrometheusExporterUtils::TranslateToPrometheus(
+    std::vector<metric_sdk::Record> &records)


Can records be a constref?

changed to a constref

g-easy · 2020-08-24T04:08:52Z

exporters/prometheus/src/prometheus_exporter_utils.cc

+/**
+ * Set value to metric family according to record
+ */
+void PrometheusExporterUtils::SetMetricFamily(metric_sdk::Record &record,


Can record be a constref?

If I make record a constref here, I need to change all getter functions in Record class const functions. Record class was implemented by previous interns, and also used in other classes.

g-easy · 2020-08-24T04:09:11Z

exporters/prometheus/src/prometheus_exporter_utils.cc

+  if (origin_name != sanitized)
+  {
+    std::cout << "Sanitized metric name \"" << origin_name << "\" to \"" << sanitized << "\""
+              << std::endl;


Please use \n over endl.

g-easy · 2020-08-24T04:11:02Z

exporters/prometheus/src/prometheus_exporter_utils.cc

+                                             std::chrono::nanoseconds time,
+                                             const std::string &labels)
+{
+  metric.timestamp_ms = time.count() / 1000;


milliseconds = nanoseconds / 1000?

fixed. thanks

g-easy · 2020-08-24T04:11:12Z

exporters/prometheus/src/prometheus_exporter_utils.cc

+      if (origin_name != sanitized)
+      {
+        std::cout << "Sanitized label name \"" << origin_name << "\" to \"" << sanitized << "\""
+                  << std::endl;


reyang · 2020-08-25T17:16:23Z

ci/do_ci.sh

+#  apt-get install zlib1g-dev
+#  apt-get -y install libcurl4-openssl-dev
+  cd third_party
+  git clone https://github.com/jupp0r/prometheus-cpp


Consider extract this logic to a separate file under the Prometheus exporter folder.

Sure, correct me if I misunderstand something:

keep the elif [[ "$1" == "cmake.exporter.prometheus.test" ]]; block here

move the scripts of installing the Prometheus client (from cd third_party to sudo make install) to a separate script in exporters/prometheus. For example, exporters/prometheus/install_prometheus_client.sh

run install_prometheus_client.sh here

reyang · 2020-08-25T17:17:21Z

ci/do_ci.sh

@@ -83,8 +110,8 @@ elif [[ "$1" == "bazel.legacy.test" ]]; then
  bazel test $BAZEL_TEST_OPTIONS -- //... -//exporters/otlp/...
  exit 0
 elif [[ "$1" == "bazel.noexcept" ]]; then
-  bazel build --copt=-fno-exceptions $BAZEL_OPTIONS //...
-  bazel test --copt=-fno-exceptions $BAZEL_TEST_OPTIONS //...
+  bazel build --copt=-fno-exceptions $BAZEL_OPTIONS -- //... -//exporters/prometheus/...


Why would we want to take exception here?

When we run the test, we found some error handling and exceptions from the Prometheus client, which made this test always fail. We discussed this problem with Johannes last week, and he suggested us to consider excluding the Prometheus component in the noexcept test.

Please add a comment here.

reyang · 2020-08-25T17:59:24Z

exporters/prometheus/src/prometheus_exporter_utils.cc

+  auto sanitized   = SanitizeNames(origin_name);
+  if (origin_name != sanitized)
+  {
+    std::cout << "Sanitized metric name \"" << origin_name << "\" to \"" << sanitized << "\"\n";


Is this intended or just for development/debugging purpose?

We added this printing in order to tell the user we have changed their metrics name or label names. We know this is not in the specification, and we can remove this.

reyang · 2020-08-25T18:00:17Z

exporters/prometheus/src/prometheus_exporter_utils.cc

+      auto sanitized   = SanitizeNames(origin_name);
+      if (origin_name != sanitized)
+      {
+        std::cout << "Sanitized label name \"" << origin_name << "\" to \"" << sanitized << "\"\n";


Same here, need to understand if we intend to keep the cout lines.

reyang

LGTM.

add Prometheus Exporter Utils build files, headers, implementations, …

f171647

…and unit test file

CunjunWang requested a review from a team August 17, 2020 23:10

This was referenced Aug 17, 2020

Add Prometheus Exporter - Utils Implementation #250

Closed

Add Prometheus Exporter - Utils Unit Tests #251

Closed

CunjunWang added 8 commits August 17, 2020 22:35

remove helper of validating name because it's guaranteed in sdk

75df0a8

format code

9c74f41

delete useless try catch

91f382a

delete useless try catch

b826383

try to add promethues client to ci

d98baf5

try to add promethues client to ci

deb2d4b

try to add promethues client to ci

c099a76

try to add promethues client to ci

215a938

CunjunWang added 2 commits August 18, 2020 11:17

modify prometheus client build

eee7fac

extract prometheus client instalation to a separate script

eb4957e

This was referenced Aug 18, 2020

Add Prometheus Exporter: Step 2 - PrometheusCollector #282

Merged

Add Prometheus Exporter: Step 3 - PrometheusExporter class #263

Closed

CunjunWang and others added 8 commits August 18, 2020 14:13

change script to use prometheus client 0.9.0 release

c1e1da3

try to add prom-client git submodule; try to add script to install on…

942714e

… windows with vcpkg

Merge branch 'master' into prometheus-exporter-utils

3a70d63

add prometheus-client as subdirectory in main cmake file

a13bf69

merge with remote

5a21c58

Removed submodule

7f8f2f5

try to install prometheus client with vcpkg

26c735a

Merge branch 'master' into prometheus-exporter-utils

019e3f5

CunjunWang added 2 commits August 21, 2020 11:11

add windows prometheus client installation

c083771

Merge branch 'prometheus-exporter-utils' of https://github.com/Cunjun…

31d5992

…Wang/opentelemetry-cpp into prometheus-exporter-utils

CunjunWang mentioned this pull request Aug 21, 2020

How-to install Prometheus Client in the C++ SDK CI Environment #291

Closed

modify prometheus client command

140c73d

g-easy reviewed Aug 24, 2020

View reviewed changes

CunjunWang and others added 12 commits August 24, 2020 09:24

stick prometheus client to v0.9.0 and added sha256

5e7453f

fix problems from review comments

0383028

change CMakefile to conditional build prometheus

645da39

modify CI bazel config so that prometheus is excluded for noexcept test

22237b4

try to fix 404 not found problem in setup cmake

1d2930e

format code

09eb665

restore setup cmake file

6e525d9

add prometheus exporter tests to ci workflow

e5f63c9

install dependencies in setup_cmake

17639bc

try to figure out install permission problem

7c6b308

Merge branch 'master' into prometheus-exporter-utils

eaceeb5

Merge branch 'master' into prometheus-exporter-utils

0d7b02c

reyang reviewed Aug 25, 2020

View reviewed changes

CunjunWang added 2 commits August 25, 2020 16:05

remove printing for sanitize names

fda7664

add comment for ignoring Prometheus Exporter in noexcept test

6569d3e

reyang approved these changes Aug 25, 2020

View reviewed changes

format code

781277d

reyang added the pr:please-merge This PR is ready to be merged by a Maintainer (rebased, CI passed, has enough valid approvals, etc.) label Aug 25, 2020

format code

c370218

erichsueh3 mentioned this pull request Aug 25, 2020

C++ SDK creates invalid metrics label name records for Prometheus #297

Closed

reyang added 2 commits August 25, 2020 18:07

Merge branch 'master' into prometheus-exporter-utils

dc4c1fe

Merge branch 'master' into prometheus-exporter-utils

3f81dee

reyang merged commit e0a93fd into open-telemetry:master Aug 26, 2020

lalitb mentioned this pull request Mar 10, 2021

Add Prometheus exporter #81

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Prometheus Exporter: Step 1 #280

Add Prometheus Exporter: Step 1 #280

CunjunWang commented Aug 17, 2020 •

edited

Loading

CunjunWang commented Aug 17, 2020

codecov bot commented Aug 18, 2020 •

edited

Loading

reyang commented Aug 21, 2020

g-easy Aug 24, 2020

CunjunWang Aug 24, 2020

g-easy Aug 24, 2020

CunjunWang Aug 24, 2020

g-easy Aug 24, 2020

CunjunWang Aug 24, 2020

g-easy Aug 24, 2020

CunjunWang Aug 24, 2020

g-easy Aug 24, 2020

CunjunWang Aug 24, 2020

g-easy Aug 24, 2020

CunjunWang Aug 24, 2020

g-easy Aug 24, 2020

CunjunWang Aug 24, 2020

reyang Aug 25, 2020

CunjunWang Aug 25, 2020

reyang Aug 25, 2020

CunjunWang Aug 25, 2020

reyang Aug 25, 2020

reyang Aug 25, 2020

CunjunWang Aug 25, 2020

CunjunWang Aug 25, 2020

reyang Aug 25, 2020

CunjunWang Aug 25, 2020

reyang left a comment

Add Prometheus Exporter: Step 1 #280

Add Prometheus Exporter: Step 1 #280

Conversation

CunjunWang commented Aug 17, 2020 • edited Loading

Changes

CunjunWang commented Aug 17, 2020

codecov bot commented Aug 18, 2020 • edited Loading

Codecov Report

reyang commented Aug 21, 2020

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

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

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

reyang left a comment

Choose a reason for hiding this comment

CunjunWang commented Aug 17, 2020 •

edited

Loading

codecov bot commented Aug 18, 2020 •

edited

Loading