Skip to content

Add Prometheus Exporter - Utils Unit Tests#251

Closed
CunjunWang wants to merge 8 commits into
open-telemetry:masterfrom
CunjunWang:prom-exporter-utils-test
Closed

Add Prometheus Exporter - Utils Unit Tests#251
CunjunWang wants to merge 8 commits into
open-telemetry:masterfrom
CunjunWang:prom-exporter-utils-test

Conversation

@CunjunWang
Copy link
Copy Markdown
Contributor

This PR implements the unit tests for utils in the exporter/prometheus/test folder for the Prometheus Exporter project. The C++ file prometheus_exporter_utils_test.cc implements all the unit test suites for the helper functions that will be called when we translate a OpenTelemetry (OTLP) metric to a Prometheus metric.

Note to reviewers:

  1. Please keep in mind that there are other PRs related to this one so codecov tests may not fully pass (yet) until other PRs are merged.
  2. The Prometheus Exporter Utils is still under development, so we will add more test cases as more code is implemented.

@CunjunWang CunjunWang requested a review from a team August 5, 2020 23:13
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 5, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@422b7a4). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #251   +/-   ##
=========================================
  Coverage          ?   92.47%           
=========================================
  Files             ?      142           
  Lines             ?     6204           
  Branches          ?        0           
=========================================
  Hits              ?     5737           
  Misses            ?      467           
  Partials          ?        0           

target_link_libraries(
${testname} ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT}
prometheus_exporter prometheus-cpp::pull)
gtest_add_tests(TARGET ${testname} TEST_PREFIX exporter. TEST_LIST ${testname})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you combine this unit tests with the piece of code that is tested into one PR? And provide Bazel build support for it, as well as hooking it into the global CMake build by adding the prometheus directory as a subdirectory to exporters/CMakeLists.txt?

I would greatly help the reviewing process to have those tests running in the CI.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, I can do that, but I am worrying that the combined PR will be so big. We have three unit test files that contain more than 1000 lines. Can we have such a large PR?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it's good to have each PR buildable, with unit tests that cover the added new functionality. If the only way to ensure that is a bigger PR, than I think it's better to have a bigger PR.

Maybe you can have three PRs: first the exporter utils with unit tests, second the collector with unit tests based on the first, and third the exporter with unit tests based on the second. We can then review and merge from first to third, rebasing later ones when previous ones got merged.

@CunjunWang
Copy link
Copy Markdown
Contributor Author

Refactored in #280

@CunjunWang CunjunWang closed this Aug 17, 2020
GerHobbelt pushed a commit to GerHobbelt/opentelemetry-cpp that referenced this pull request Jun 17, 2025
chore(deps): update dependency gazelle to v0.43.0
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.

2 participants