Skip to content
This repository has been archived by the owner on Jul 31, 2023. It is now read-only.

Add cmake support for prometheus exporter #249

Conversation

StevenYCChou
Copy link
Contributor

@StevenYCChou StevenYCChou commented Dec 5, 2018

To solve #248

- disable building pull/push libraries
- disable compression to avoid pulling dependent gzip library
- disable test so it would not need to build prometheus-cpp tests
@g-easy g-easy mentioned this pull request Dec 5, 2018
16 tasks
@g-easy
Copy link
Contributor

g-easy commented Dec 5, 2018

This looks great!

It builds and passes tests.

I tried to add the prometheus exporter to helloworld but we're missing exposer.h from prometheus-cpp::pull. You can see my branch here: https://github.com/g-easy/opencensus-cpp/commits/pr249

Would you like to handle pull as part of this PR, or in a followup?

Copy link
Contributor

@g-easy g-easy left a comment

Choose a reason for hiding this comment

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

Also, please run tools/format.sh :)

cmake/OpenCensusDeps.cmake Outdated Show resolved Hide resolved
cmake/OpenCensusDeps.cmake Outdated Show resolved Hide resolved
cmake/prometheus-cpp.CMakeLists.txt Outdated Show resolved Hide resolved
opencensus/exporters/stats/prometheus/CMakeLists.txt Outdated Show resolved Hide resolved
opencensus/exporters/stats/prometheus/CMakeLists.txt Outdated Show resolved Hide resolved
cmake/OpenCensusDeps.cmake Outdated Show resolved Hide resolved
@StevenYCChou
Copy link
Contributor Author

Let's create another PR as this PR is self-contained. However, I identified a line too long from your commit, so I will push another commit to make the format right.

@g-easy
Copy link
Contributor

g-easy commented Dec 5, 2018

tools/format.sh caught the long line ;)

Thanks for taking care of it.

@g-easy
Copy link
Contributor

g-easy commented Dec 5, 2018

Agreed, let's leave prometheus-cpp::pull to another PR. Would you like to address the other comments in this PR?

@StevenYCChou
Copy link
Contributor Author

I will address the issue you raise and follow up with you later - just saw your comments after I pushed that commit.

@StevenYCChou
Copy link
Contributor Author

Do I need to rebase from master, or merge the latest branch from master? Or are you able to squash & merge them into single commit? I believe there won't be conflict.

@g-easy
Copy link
Contributor

g-easy commented Dec 5, 2018

We always squash in opencensus-cpp.

Please:

  • Update branch.
  • Click "Squash and merge."
  • Please clean up the commit message from github's suggestion before completing the merge.

Thanks :)

@StevenYCChou
Copy link
Contributor Author

As I don't have write access, so I cannot merge back to master.
I will leave the commit message in my mind here for squashed commit and I hope it's useful for you:

Add cmake support for prometheus exporter.

This resolved only part of #249, because the 
build rule for prometheus_test_server is not added yet.  

@g-easy g-easy merged commit f449234 into census-instrumentation:master Dec 5, 2018
@g-easy
Copy link
Contributor

g-easy commented Dec 5, 2018

Thanks!

@StevenYCChou StevenYCChou deleted the ycchou-add-prometheus-exporter-cmake branch December 6, 2018 05:32
meastp pushed a commit to meastp/opencensus-cpp that referenced this pull request Feb 19, 2019
This resolved only part of census-instrumentation#249, because the 
build rule for prometheus_test_server is not added yet.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants