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

Add Prometheus Exporter #1031

Merged
merged 83 commits into from
Nov 9, 2021
Merged

Conversation

esigo
Copy link
Member

@esigo esigo commented Oct 24, 2021

This PR attempts to finish #263

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

erichsueh3 and others added 30 commits August 6, 2020 18:34
@codecov
Copy link

codecov bot commented Oct 24, 2021

Codecov Report

Merging #1031 (8ff4d7a) into main (c3f3042) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1031   +/-   ##
=======================================
  Coverage   94.85%   94.85%           
=======================================
  Files         151      151           
  Lines        5971     5971           
=======================================
  Hits         5663     5663           
  Misses        308      308           

@esigo esigo marked this pull request as ready for review October 27, 2021 19:08
@esigo esigo requested a review from a team October 27, 2021 19:08
Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

Looks good in general.

*/

OPENTELEMETRY_BEGIN_NAMESPACE
namespace sdkmetrics = opentelemetry::sdk::metrics;
Copy link
Member

@lalitb lalitb Nov 4, 2021

Choose a reason for hiding this comment

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

nit: not recommended to use the namespace alias in the header file, as it becomes part of public API. It's been used all over the code right now and #1008 should be fixing it.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

CHANGELOG.md Outdated
@@ -19,6 +19,7 @@ Increment the:

## [1.0.1] 2021-10-21

* [EXPORTER] Prometheus Exporter ([#1031](https:/open-telemetry/opentelemetry-cpp/pull/1031))
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be put into the above section for "Unreleased".

Copy link
Member Author

Choose a reason for hiding this comment

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

done

ci/do_ci.sh Outdated
@@ -85,6 +84,7 @@ elif [[ "$1" == "cmake.c++20.stl.test" ]]; then
make test
exit 0
elif [[ "$1" == "cmake.legacy.test" ]]; then

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove this empty line.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

ci/do_ci.sh Outdated
@@ -93,6 +93,7 @@ elif [[ "$1" == "cmake.legacy.test" ]]; then
cmake -DCMAKE_BUILD_TYPE=Debug \
-DCMAKE_CXX_FLAGS="-Werror" \
-DCMAKE_CXX_STANDARD=11 \
-DCMAKE_EXE_LINKER_FLAGS="-lpthread" \
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this required?

Copy link
Member Author

Choose a reason for hiding this comment

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

was not needed. cleaned

@lalitb
Copy link
Member

lalitb commented Nov 5, 2021

@esigo - Sorry about this, but realized afterward that the #1053 merge will break this PR. Unfortunately, we need changes in this PR for CI to go through ( I can do it if you want in your branch as my PR was the culprit to break this ).

@@ -15,16 +15,13 @@
include_directories(include)
find_package(prometheus-cpp CONFIG REQUIRED)

add_library(prometheus_exporter_deprecated src/prometheus_collector.cc
Copy link
Contributor

Choose a reason for hiding this comment

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

The target is removed here, but I didn't see it is added back in other places but it is referenced below in target_include_directories.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@esigo
Copy link
Member Author

esigo commented Nov 5, 2021

@esigo - Sorry about this, but realized afterward that the #1053 merge will break this PR. Unfortunately, we need changes in this PR for CI to go through ( I can do it if you want in your branch as my PR was the culprit to break this ).
@lalitb No problem. I'll take care of it :)

@@ -0,0 +1,111 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Simplify the copyright header as below?

// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@lalitb lalitb merged commit 994c08a into open-telemetry:main Nov 9, 2021
@esigo esigo deleted the esigo-prometheus-exporter branch November 9, 2021 16:30
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.

4 participants