Add Jaeger exporter to CMake CI build #786
Conversation
Codecov Report
@@ Coverage Diff @@
## main #786 +/- ##
=======================================
Coverage 96.00% 96.00%
=======================================
Files 176 176
Lines 7183 7183
=======================================
Hits 6896 6896
Misses 287 287 |
| -DBUILD_COMPILER=OFF \ | ||
| -DBUILD_CPP=ON \ | ||
| -DBUILD_LIBRARIES=ON \ | ||
| -DBUILD_NODEJS=OFF \ |
There was a problem hiding this comment.
All the options turned off here are turned on by default in thrift repo. They are not used by our C++ build so turn them off explicitly would introducing more dependence to our CI build.
| @@ -19,7 +19,9 @@ jobs: | |||
| sudo ./ci/setup_cmake.sh | |||
| sudo ./ci/setup_ci_environment.sh | |||
| - name: run cmake tests (without otlp-exporter) | |||
| run: ./ci/do_ci.sh cmake.test | |||
| run: | | |||
| sudo ./ci/setup_thrift.sh | |||
There was a problem hiding this comment.
We don't need sudo, as github actions are run by default as root user.
Also, if we move thrift installation as part of do_ci.sh script, this script will remain usable outside of github workflow, without separately installing thrift ( similar to how it is done for prometheus). Probably setup_thrift.sh can expose a function to install thrift, and that function can be called from do_ci.sh for actual installation if called with "cmake.test" argument.
There was a problem hiding this comment.
I also believe sudo is not necessary, just follow other scripts invocation like setup_grpc.h.
I don't prefer moving the installation logic to do_ci.sh which would make do_ci.sh too complicated with the third-party dependence setup instructions, and also hard to share with non-CI environment. Expose a setup function in setup_thrift.sh looks good, but we still need include setup_thrift.sh to invoke the function. So it may be simpler to just invoke the script?
There was a problem hiding this comment.
@lalitb do all github action run as root by default? I just removed all the sudo in our ci.yml but got permission denied in the CI run.
There was a problem hiding this comment.
I thought unless explicitly specified with --user options, all jobs run as root user. Probably revert it back if there is a permission denied error. Sorry for the confusion.
Fixes #785
Changes
Follow the Zipkin exporter, this PR add Jaeger exporter to CMake CI build without OTLP exporter. OTLP exporter takes a lot of time to build dependencies, group all non-OTLP exporters in a separate CI build will reduce the total CI build time.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes