Add Prometheus Exporter: Step 3 - PrometheusExporter class #263

erichsueh3 · 2020-08-07T01:38:20Z

This PR implements the PrometheusExporter class and its header, source, test, and build files. This class is derived from the Metrics Exporter Interface and allows for export of Prometheus metrics data.

This PR is one in a series of PRs that aims to add a full implementation of a Prometheus Exporter within the OpenTelemetry C++ library.

Note to reviewers: Please keep in mind that this PR is dependent on the Metrics Exporter Interface PR (#240), the Prometheus Collector Implementation PR (#282 ), and the Prometheus Exporter Utils PRs (#280) so codecov tests may not fully pass.

codecov · 2020-08-07T01:43:18Z

Codecov Report

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

@@           Coverage Diff           @@
##           master     #263   +/-   ##
=======================================
  Coverage   94.59%   94.59%           
=======================================
  Files         146      146           
  Lines        6629     6629           
=======================================
  Hits         6271     6271           
  Misses        358      358

… kFailureTimeout

…emetry-cpp into prom_exporter

exporters/prometheus/src/prometheus_exporter.cc

g-easy · 2020-08-31T07:41:28Z

exporters/prometheus/src/prometheus_exporter.cc

+ */
+void PrometheusExporter::Shutdown() noexcept
+{
+  is_shutdown_ = true;


Is Shutdown called from a different thread? Should there be a lock here?

After looking into it, it seems as though Shutdown() will never be called by the controller in the SDK, since the Metrics Exporter Interface does not require a shutdown function - is there any benefit to keeping this in? If yes, then it seems we might need a lock here.

What does the specification say about exporter shutdowns?

It seems that it is not required for metrics exporters currently, but is required for span exporters. Here is the requirement for span exporters, but looking at the shutdown function in the ostream metrics exporter, it is mentioned but not required.

At some point this might be nice to have. I think Ryan was saying that e.g. Envoy wants to be able to handle SIGHUP by completely shutting everything down, changing the config, and then bringing it back up.

Maybe the right answer is to punt on Shutdown for now and just leave a TODO? Idk.

Maybe we can comment the shutdown function out, and add a TODO comment once the metrics specification adds a requirement for a shutdown?

This reverts commit d3d44f3.

erichsueh3 · 2020-09-03T07:13:07Z

@reyang The CI/CMake test isn't passing the check here- the unit tests for this class all fail, and after further testing, it seems as if they fail independent of the actual outcome of the test. There seems to be a problem within the CI setup but after trying to debug it for the better part of a day I cannot seem to reproduce the error locally, so I was wondering if you might have any insight into this. Additionally, the bazel valgrind test fails but it seems as if this is due to something within the otlp exporter test, so I think we can safely ignore this.

lalitb · 2020-09-03T15:32:26Z

@reyang The CI/CMake test isn't passing the check here- the unit tests for this class all fail, and after further testing, it seems as if they fail independent of the actual outcome of the test. There seems to be a problem within the CI setup but after trying to debug it for the better part of a day I cannot seem to reproduce the error locally, so I was wondering if you might have any insight into this. Additionally, the bazel valgrind test fails but it seems as if this is due to something within the otlp exporter test, so I think we can safely ignore this.

@erichsueh3 prometheus-cpp library uses C++11 thread model, so you need to add pthread library in cmake link option.
i.e, Add MAKE_EXE_LINKER_FLAGS="-lpthread" here

erichsueh3 · 2020-09-03T17:24:23Z

@reyang The CI/CMake test isn't passing the check here- the unit tests for this class all fail, and after further testing, it seems as if they fail independent of the actual outcome of the test. There seems to be a problem within the CI setup but after trying to debug it for the better part of a day I cannot seem to reproduce the error locally, so I was wondering if you might have any insight into this. Additionally, the bazel valgrind test fails but it seems as if this is due to something within the otlp exporter test, so I think we can safely ignore this.

@erichsueh3 prometheus-cpp library uses C++11 thread model, so you need to add pthread library in cmake link option.
i.e, Add MAKE_EXE_LINKER_FLAGS="-lpthread" here

@lalitb thanks for the suggestion, I've made the changes but unfortunately it seems like I'm still running into the same problem.

lalitb · 2020-09-03T18:33:14Z

@reyang The CI/CMake test isn't passing the check here- the unit tests for this class all fail, and after further testing, it seems as if they fail independent of the actual outcome of the test. There seems to be a problem within the CI setup but after trying to debug it for the better part of a day I cannot seem to reproduce the error locally, so I was wondering if you might have any insight into this. Additionally, the bazel valgrind test fails but it seems as if this is due to something within the otlp exporter test, so I think we can safely ignore this.

@erichsueh3 prometheus-cpp library uses C++11 thread model, so you need to add pthread library in cmake link option.
i.e, Add MAKE_EXE_LINKER_FLAGS="-lpthread" here

@lalitb thanks for the suggestion, I've made the changes but unfortunately it seems like I'm still running into the same problem.

@erichsueh3 Seems issue is easily reproducible with below set of commands locally from root directory of your repo clone ( i.e, running test within ubuntu docker container )

labhas@1dscpp:~/ot/eric/opentelemetry-cpp$ ./ci/run_docker.sh
root@fabee4b43e6e:/src# ./ci/do_ci.sh cmake.exporter.prometheus.test

98% tests passed, 6 tests failed out of 306

Can you try debugging using these steps in case this is test case issue, else I will look into it.

erichsueh3 · 2020-09-03T20:21:23Z

@reyang The CI/CMake test isn't passing the check here- the unit tests for this class all fail, and after further testing, it seems as if they fail independent of the actual outcome of the test. There seems to be a problem within the CI setup but after trying to debug it for the better part of a day I cannot seem to reproduce the error locally, so I was wondering if you might have any insight into this. Additionally, the bazel valgrind test fails but it seems as if this is due to something within the otlp exporter test, so I think we can safely ignore this.

@erichsueh3 prometheus-cpp library uses C++11 thread model, so you need to add pthread library in cmake link option.
i.e, Add MAKE_EXE_LINKER_FLAGS="-lpthread" here

@lalitb thanks for the suggestion, I've made the changes but unfortunately it seems like I'm still running into the same problem.

@erichsueh3 Seems issue is easily reproducible with below set of commands locally from root directory of your repo clone ( i.e, running test within ubuntu docker container )
labhas@1dscpp:~/ot/eric/opentelemetry-cpp$ ./ci/run_docker.sh
root@fabee4b43e6e:/src# ./ci/do_ci.sh cmake.exporter.prometheus.test

98% tests passed, 6 tests failed out of 306
Can you try debugging using these steps in case this is test case issue, else I will look into it.

Seems like I was able to reproduce this locally now, and it still seems that regardless of what a test case's body is, the test will fail. For example, a test body that looks like

bool b = true;
ASSERT_TRUE(b);

still fails.

erichsueh3 · 2020-09-04T17:02:22Z

@reyang @lalitb unfortunately, my internship ends today - I'll keep debugging today and hopefully figure this out

erichsueh3 · 2020-09-04T21:20:43Z

@reyang @lalitb unfortunately, my internship ends today - I'll keep debugging today and hopefully figure this out

Was not able to make any headway on this, if you have any more ideas please let me know

lalitb · 2020-09-05T01:19:35Z

@erichsueh3 no worries I will look into it further. If there are further pending PRs dependend on this one, you can comment out prometheus test cpp from cmake so that these tests are ignored for now. This will allow you for merge request. We will debug and add it back once fixed. Thanks for your contributions :)

erichsueh3 · 2020-09-05T05:15:33Z

@lalitb thanks for the help! The only PR that depends on this one is the example program (#310), and shouldn't be merged without this one being merged first anyways, so seems as if there won't be any need to comment anything out

compscidr · 2021-10-08T17:22:57Z

Sorry to post on an old / closed issue, just curious if there is any interest in getting this across the finish line? Looks like its abandoned from an internship ending. I know in one of the discussions (#952) it was mentioned that metrics was still be redesigned - but in the meantime, its been useful to us to have a Prometheus exporter. We're actually using code based on this PR currently.

I / our team at @rapidrobotics would be able to work on any changes needed to get this in and support it if there is interest. Happy to jump on a call to discuss what needs to be done if so. Also happy going forward to make adjustment as needed if / when the metrics stuff does change.

lalitb · 2021-10-09T16:42:11Z

@compscidr metrics specs is now stable, and implementation work is ongoing. Feel free to join the community meeting and we can discuss further.

lalitb · 2021-10-21T00:47:57Z

@esigo Seems I can't reopen this PR as it is targeted to the old master branch, and we are using the main branch. Also can't edit the target branch. You may have to create a new one taking these changes.

Add PrometheusExporter header, source, and test files

38f811a

erichsueh3 requested a review from a team August 7, 2020 01:38

erichsueh3 added 2 commits August 7, 2020 10:51

CI formatting, added copyright headers

140efcd

Added build files

feb7126

erichsueh3 changed the title ~~Add Prometheus Exporter - PrometheusExporter header, source, and tests~~ Add Prometheus Exporter: Step 3 - PrometheusExporter class Aug 18, 2020

modified comments to reflect removal of ReturnCodes class, removal of…

5d6fc7e

… kFailureTimeout

erichsueh3 mentioned this pull request Aug 25, 2020

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

Closed

erichsueh3 and others added 4 commits August 28, 2020 17:01

Merge branch 'master' into prom_exporter

845d6f1

minor change to structure of friend class

32880b0

Merge branch 'prom_exporter' of https://github.com/erichsueh3/opentel…

61783a7

…emetry-cpp into prom_exporter

Merge branch 'master' into prom_exporter

b8d8f5a

g-easy reviewed Aug 31, 2020

View reviewed changes

removed assignment of exposer_ to nullptr in testing constructor

94af70d

CunjunWang mentioned this pull request Aug 31, 2020

Add C++ Prometheus Exporter Example Program #310

Closed

erichsueh3 and others added 7 commits September 2, 2020 11:01

Merge branch 'master' into prom_exporter

550bb7f

removed usage of release9.0 of Prometheus

367d6a3

BUILD file changes

a5e2cfb

changed Prometheus client back to 9.0

6bbb051

removed a test for debugging

d3d44f3

Revert "removed a test for debugging"

8dc5cb1

This reverts commit d3d44f3.

format

94f78c8

erichsueh3 added 3 commits September 3, 2020 09:49

added MAKE_EXE_LINKER_FLAGS cmake link option

5938aa8

added -D to flag

dc28f84

changed -DMAKE_EXE_LINKER_FLAGS to -DCMAKE_EXE_LINKER_FLAGS

75e23e5

reyang closed this Oct 7, 2020

lalitb mentioned this pull request Jul 19, 2021

Not able to find PrometheusExporter class in Prometheus Exporter #908

Closed

compscidr mentioned this pull request Oct 8, 2021

metrics: add prometheus exporter rapidrobotics/opentelemetry-cpp#1

Closed

esigo mentioned this pull request Oct 24, 2021

Add Prometheus Exporter #1031

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Prometheus Exporter: Step 3 - PrometheusExporter class #263

Add Prometheus Exporter: Step 3 - PrometheusExporter class #263

erichsueh3 commented Aug 7, 2020 •

edited

Loading

codecov bot commented Aug 7, 2020 •

edited

Loading

g-easy Aug 31, 2020

erichsueh3 Aug 31, 2020

g-easy Sep 3, 2020

erichsueh3 Sep 3, 2020

g-easy Sep 3, 2020

erichsueh3 Sep 3, 2020

erichsueh3 commented Sep 3, 2020

lalitb commented Sep 3, 2020

erichsueh3 commented Sep 3, 2020

lalitb commented Sep 3, 2020

erichsueh3 commented Sep 3, 2020

erichsueh3 commented Sep 4, 2020

erichsueh3 commented Sep 4, 2020

lalitb commented Sep 5, 2020

erichsueh3 commented Sep 5, 2020

compscidr commented Oct 8, 2021 •

edited

Loading

lalitb commented Oct 9, 2021

lalitb commented Oct 21, 2021

Add Prometheus Exporter: Step 3 - PrometheusExporter class #263

Add Prometheus Exporter: Step 3 - PrometheusExporter class #263

Conversation

erichsueh3 commented Aug 7, 2020 • edited Loading

codecov bot commented Aug 7, 2020 • edited Loading

Codecov Report

g-easy Aug 31, 2020

Choose a reason for hiding this comment

erichsueh3 Aug 31, 2020

Choose a reason for hiding this comment

g-easy Sep 3, 2020

Choose a reason for hiding this comment

erichsueh3 Sep 3, 2020

Choose a reason for hiding this comment

g-easy Sep 3, 2020

Choose a reason for hiding this comment

erichsueh3 Sep 3, 2020

Choose a reason for hiding this comment

erichsueh3 commented Sep 3, 2020

lalitb commented Sep 3, 2020

erichsueh3 commented Sep 3, 2020

lalitb commented Sep 3, 2020

erichsueh3 commented Sep 3, 2020

erichsueh3 commented Sep 4, 2020

erichsueh3 commented Sep 4, 2020

lalitb commented Sep 5, 2020

erichsueh3 commented Sep 5, 2020

compscidr commented Oct 8, 2021 • edited Loading

lalitb commented Oct 9, 2021

lalitb commented Oct 21, 2021

erichsueh3 commented Aug 7, 2020 •

edited

Loading

codecov bot commented Aug 7, 2020 •

edited

Loading

compscidr commented Oct 8, 2021 •

edited

Loading