Integrate prometheus-enabled Broker #3794
Conversation
Neverlord
force-pushed
the
topic/neverlord/prometheus-cpp
branch
6 times, most recently
from
f1f3254
to
a1900ab
Compare
Neverlord
force-pushed
the
topic/neverlord/prometheus-cpp
branch
from
a1900ab
to
f1d65b7
Compare
|
I've now played with this for a bit and I'm still pretty uneasy. A few questions/observations:
Unless I'm missing something, some of what informed this new approach doesn't really matter for Zeek: it doesn't care about freedom of choice around Prometheus installations (such as picking a system-level one), since we're already pinning our own fork. The complexity comes from Broker's approach to choosing Prometheus, and the general desire to encapsulate its configuration & build as a third party. And that latter part is clearly appealing and relevant for Zeek too, as we've discussed before. Given that we're just about to fork Zeek 7, I'm inclined to be more conservative. Specifically, I am wondering whether we should treat Broker as a black box and confine the experimentation around this new way of building third-party components to it, for now. It is clearly interesting, might well matter for folks who build Broker separately, and will inform how we do this going forward. But as I understand it, we can keep Zeek's approach to configuring and building Prometheus just as it is right now, at the expense of two statically linked builds of Prometheus ending up in Zeek — one in the executable, one in the Broker Am I roughly on track here? I'd really value your inputs. |
For me, it is using the We have the Broker setup immediately after diff --git a/CMakeLists.txt b/CMakeLists.txt
index 14be7998f..e636bc6e4 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -911,6 +911,8 @@ zeekbundle_add(
target_link_libraries(zeek_internal INTERFACE $<BUILD_INTERFACE:prometheus-cpp::core>
$<BUILD_INTERFACE:prometheus-cpp::pull>)
+message(STATUS "prometheus-cpp setup done")
+
# Note: Broker gets some special attention in ZeekConfig.cmake.in.
if (Broker_ROOT)
find_package(Broker REQUIRED CONFIG)Then it'll print the extra line before the line regarding
That is intentional and how @timwoj did it before. I believe the reason was because civetweb otherwise spams stuff to stdout and there's no way to disable the output other than doing a release build.
I could add a btest for that.
Looking at the script, it'll set
I can't follow your train of thought here. Zeek decided to bundle prometheus-cpp (and fork it), not Broker. Maybe I misinterpret your statement, but we need to pass down prometheus-cpp to Broker in order to make sure it has the same library version, not the other way around. The Broker manager passes a prometheus registry object to Broker in order to have all metrics in the same registry. This is why we need to make sure we're having the same library version. If Broker would keep its own metrics (or none), we would no longer have that dependency.
I'm not sure if I can follow you here. Broker has no separate way of bundling third-party components. So far, it only had to deal with dependencies that we either don't bundle (OpenSSL) or that are easy to pull in via sub-directory. The way Zeek currently integrates prometheus-cpp into the build breaks CMake target exports, which in turn breaks Broker if we try to consume the prometheus-cpp target that Zeek has defined. The ZeekBundle script is an attempt to fix that by having imported targets for the prometheus-cpp library and than have both Zeek and Broker consume the static library from the build tree. Do you mean "treat Broker as a black box" in the sense that Zeek and Broker metrics shouldn't go into the same registry? That would make the problem go away, since only Zeek would care about prometheus-cpp in that case. |
|
Thanks Dominik, helpful! I see the same output as you, so it looks like I misread the output. Let me focus on the second half, after my complexity comment. I do mean for Broker to use Zeek's Prometheus registry. I meant "black box" in the sense that we leave it up to Broker how it manages its Prometheus build, and change nothing in Zeek other than the C++ tweak to pass the registry into Broker. I also mean black box at a higher level, because I'd like us to start treating Broker as an independently maintained third-party project — or at least consider what we would should do to treat it as one — because I think it will ensure better modularity around it going forward. I am wondering the following: what if we use Broker as the testing ground for the new approach to building third-party modules, but do so only there for now. It can use the ZeekBundle/FetchContent approach, and benefit from the flexibility of choosing Prometheus installs in standalone builds. Zeek would continue to manage Prometheus as it does right now (a pinned version of our fork, handled directly as a submodule). The downside only seems to be that we'd build Prometheus twice, once in Broker, once in Zeek, instead of sharing the bundle build (a build takes ~10s here, so that's no biggie), and we'd need to make sure those two versions are sufficiently compatible, but that seems easy. Is that feasible or am I overlooking something there? I want to stress again that I acknowledge the need for a better way to build our third-party stuff. I just don't like the idea of doing this in Zeek at configure time for this one project. It feels like something we should give more thought over the 7.1 timeframe. |
|
Dominik it's perhaps easier to just show you what I mean, see here: #3822 — this is essentially the content of your branch, with Broker always using Btw good news, I can confirm it works (with either version) — I see Broker's telemetry when scraping. 👍 |
Isn't this in the end only amplifying the pain? You've been concerned that changing something in the prometheus-cpp folder doesn't rebuild automatically before, but with this change, you would have to go into the Broker subdirectory, update the prometheus-cpp submodule there, then trigger a rebuild of that. Otherwise, Zeek and Broker will end up having different versions of the same library and if you don't know exactly what you're doing, you are in for one terrible debugging sessions if the ABIs clash. And that's aside form having two different setups for the same 3rd-party library, only increasing maintenance burden. If you are uncomfortable with the Since then, we have made a private fork of the library and made it incompatible with the upstream version and we are now installing header files for prometheus-cpp into the Zeek install prefix. So we could argue the original assumptions from a couple months ago no longer hold anyways. I could try to see if we can either tell the submodule to behave in the way we want it to or patch the submodule CMake if we can't get it to behave otherwise. The end result will probably be installing the |
|
The main problem we have right now is that we don't have time to resolve a new approach for this prior to forking Zeek 7, and I am admittedly just nervous about putting it in Zeek right now. I think a hybrid has benefits — minimally, it gets the bundle code some build exposure. The fact that a change of the sources needs a re-configure is a risk of the approach, so we will see how it goes. If you are up for it, by all means investigate what it'll take to make our prometheus-cpp fork play nicely in Broker. Depending on what you find, perhaps we can still switch it over. It would be my preferred choice for this point in time. In the meantime I have put up three PRs with a light edit pass over your branches (keeping you as main author of course):
I have a question for you in #3822 that may buy you some time. :-) To be clear, I think the bundle approach is neat and we might well end up using it more — we will see. Also, it is not your fault that it's now late in the game! Your PRs have been up for a long time. Apologies for that. |
|
I'm closing this as it was superseded by #3822 — here too: thanks Dominik! |
Zeek-side integration of zeek/broker#402 (and zeek/cmake#115).