zeek
Add prometheus-cpp, drop legacy telemetry API
Migrate to the prometheus-cpp API for compatibility with Zeek.
Closes #400.
@timwoj, @ckreibich: aside from getting CI green:
I sat down with prometheus-cpp to figure out how to best integrate the build. The way it has been integrated in Zeek has several drawbacks that I'd like to address. On the one hand, Zeek still relies on manipulating global CMake state and global variables. Just reverting the CMake build flags variable did the trick for stopping the embedded civetweb from printing debug information to console when building Zeek/Broker as a debug version. However, that only takes care of a fraction of things that can affect the prometheus-cpp build and it's very fragile. On the other hand, prometheus-cpp also isn't built for embedding it as a sub-module in CMake. It manipulates global state as well and using the defined targets without some CMake hackery will result in Zeek/Broker installing prometheus-cpp alongside our own targets. That something that we must not do, since it can easily break system installations. Several distributions have prometheus-cpp packages.
Finally, package maintainers should be able to tell Zeek/Broker to use the system dependency for prometheus-cpp if there's a package available. Just using a prometheus-cpp packages is the cleanest solution. However, so far Zeek/Broker are standalone and I think there are no plans to rely on a package manager (like Conan).
Long story short, I sat down last week to experiment with a couple ways to integrate prometheus-cpp such that it's also not just a one-off solution. I have written a small ZeekBundle.cmake script that I would propose for integrating external libraries such as prometheus-cpp in Zeek and Broker.
In Broker, this is how we currently call it:
ZeekBundle_Add(
NAME prometheus-cpp
FETCH
GIT_REPOSITORY https://github.com/jupp0r/prometheus-cpp.git
GIT_TAG v1.2.4
CONFIGURE
ENABLE_PUSH=OFF
ENABLE_TESTING=OFF
CMAKE_POSITION_INDEPENDENT_CODE=ON)
It builds on top of FetchContent. Since we have a Git sub-module for it in Zeek, the syntax there would be:
ZeekBundle_Add(
NAME prometheus-cpp
FETCH
SOURCE_DIR auxil/prometheus-cpp
CONFIGURE
ENABLE_PUSH=OFF
ENABLE_TESTING=OFF
CMAKE_POSITION_INDEPENDENT_CODE=ON)
When using this method, CMake will build the bundled library at configure time and independently. In other words, CMake will launch a separate cmake process, build the thing and install it into a custom prefix in the build tree. From there, we pick it up as we would pick up any other dependency via find_package. We will build the dependency as a static library so that we won't have a run-time dependency.
In addition, you can use the usual CMake method to have it pick up a library version from the system instead by setting the CMake variable prometheus-cpp_ROOT.
I'm happy to discuss this approach with you guys and iterate on it if you find some shortcomings (unless you have better ideas, of course). 🙂
I definitely like the idea, especially if we can use it for other submodules as well. I've never really liked how we include various subprojects in Zeek via CMake, and making how they're handled done in a common way would be a big improvement. I'm torn on the idea of things being built during configure time since it will make that drag on a bit longer, but it's probably not a big deal.
without some CMake hackery will result in Zeek/Broker installing prometheus-cpp alongside our own targets
Ideally all of our internal builds are linked statically, which the prometheus-cpp libraries should already be. I double-checked with the existing setup. Only static libraries are being generated, and they're not getting installed as part of make install.
All right, I've sat down with CMake some more to get the ZeekBundle work on Windows (cannot mix debug and release libraries) and with multi-configuration generators. I've tested it on MacOS, Linux (Fedora) and Windows (using ninja as we do on CI as well as using the default CMake generator that will emit a VisualStudio project with multi-configuration mode). Seems to work quite nicely now.
@timwoj feel free to play around with it and to integrate it into your telemetry rework branch. You can pass a shared pointer to the Prometheus registry to the broker::endpoint to have it feed into the same registry as Zeek.
Ideally all of our internal builds are linked statically, which the prometheus-cpp libraries should already be. I double-checked with the existing setup. Only static libraries are being generated, and they're not getting installed as part of make install.
It's simpler for binary targets like Zeek. All you need to do is passing EXCLUDE_FROM_ALL to add_subdirectory to suppress the install targets. However, Broker exports library targets for installation. Just pulling in prometheus-cpp via add_subdirectory will have prometheus-cpp::core point to an actual library target that isn't marked for export. CMake won't like that and will run into this error (CMake 3.29.1 on macOS):
CMake Error in auxil/broker/CMakeLists.txt:
export called with target "broker-prometheus-cpp" which requires target
"core" that is not in any export set.
CMake Error in auxil/broker/CMakeLists.txt:
export called with target "broker-prometheus-cpp" which requires target
"pull" that is not in any export set.
That's what I meant with being cautious and I can't get Zeek to build when using your topic/timw/prometheus-cpp-3 branch in Zeek with the prometheus-cpp version of Broker. There are probably some ways around this, but it's probably going to be hacky (and fragile). At least to my knowledge there's no clean way to tell CMake to "ignore" a transitive dependency in an export set. Happy to be taught otherwise, though. In any case, I'd rather find a solution that works with CMake instead of fighting against it and I think isolating the build of 3rd-party dependency is a big plus as well.
This is the patch I've used on your branch to use the ZeekBundle approach and get it building with prometheus-cpp. Of course, ZeekBundle.cmake would need to move to our cmake sub-module if we decide to use it.
$ git diff
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 60de8b05f..e1a95a2a8 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -892,6 +892,21 @@ if (MSVC)
endif ()
set(zeekdeps ${zeekdeps} paraglob)
+
+include(auxil/broker/ZeekBundle.cmake)
+ZeekBundle_Add(
+ NAME prometheus-cpp
+ FETCH
+ SOURCE_DIR "${PROJECT_SOURCE_DIR}/auxil/prometheus-cpp"
+ CONFIGURE
+ ENABLE_COMPRESSION=OFF
+ ENABLE_PUSH=OFF)
+target_link_libraries(
+ zeek_internal
+ INTERFACE
+ $<BUILD_INTERFACE:prometheus-cpp::core>
+ $<BUILD_INTERFACE:prometheus-cpp::pull>)
+
# Note: Broker gets some special attention in ZeekConfig.cmake.in.
if (Broker_ROOT)
find_package(Broker REQUIRED CONFIG)
@@ -1124,7 +1139,6 @@ include(GetArchitecture)
include(FindCAres)
include(FindKqueue)
-include(FindPrometheusCpp)
include_directories(BEFORE "auxil/out_ptr/include")
if ((OPENSSL_VERSION VERSION_EQUAL "1.1.0") OR (OPENSSL_VERSION VERSION_GREATER "1.1.0"))
diff --git a/auxil/broker b/auxil/broker
index 48660b74a..57a953683 160000
--- a/auxil/broker
+++ b/auxil/broker
@@ -1 +1 @@
-Subproject commit 48660b74ae2e5b5babd6f0dc38f3c85e24434d77
+Subproject commit 57a953683c3569223166c8023abc7e41284a2969
Hi @Neverlord, this currently does not configure against Zeek's master since it does not export the prometheus targets (see https://github.com/zeek/broker/pull/402#discussion_r1604392006); could you adjust the PR for that? I am still not a fan for sneaking FetchContent into a Zeek project (https://github.com/zeek/broker/pull/402#discussion_r1604388312), but wouldn't block this as long as it is only for broker standalone builds. Anything else you have in mind before this can graduate out of draft status?
@ckreibich the CI failures have nothing to do with the changes. PR-wise, this is done now. The Zeek-side PR is also green now.
@ckreibich the CI failures have nothing to do with the changes. PR-wise, this is done now. The Zeek-side PR is also green now.
I retriggered the image creation for the fedora job and it now succeeds. For freebsd13 you need 25ec1e87a56ba6648826cecc5442d28078503172 which should be on the master branch, can you rebase (would also resolved the merge conflicts)?
