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

Integrate prometheus-enabled Broker #3794

Closed
wants to merge 1 commit into from

Conversation

Neverlord
Copy link
Member

Zeek-side integration of zeek/broker#402 (and zeek/cmake#115).

@Neverlord Neverlord force-pushed the topic/neverlord/prometheus-cpp branch 6 times, most recently from f1f3254 to a1900ab Compare June 28, 2024 13:33
@Neverlord Neverlord force-pushed the topic/neverlord/prometheus-cpp branch from a1900ab to f1d65b7 Compare June 28, 2024 15:37
@ckreibich
Copy link
Member

I've now played with this for a bit and I'm still pretty uneasy. A few questions/observations:

  • The build of Prometheus currently happens during Broker's configuration phase, not Zeek's. Is that just coincidence because we configure Broker first? Broker's choice of Prometheus must not dictate Zeek's, because in the pretty near future we're going to regularly have builds (however experimental) that don't use Broker.
  • It would be nice if during Zeek's own configuration stage there were some output indicating which Prometheus version it's choosing for the build. Maybe I'm missing it, but I'm not seeing any?
  • When I kick off a debug build, Prometheus still does a Release build. Is that intentional? How would I control how it's built?
  • It'd be good to have a Zeek-level test that verifies that it sees Broker telemetry.
  • At least according to our configure script, we can currently do ./configure --with-broker=... — but when I try that, it's ignored and the bundled Broker continues to be used. It's possible that this was broken before, I'm not sure.
  • When I complete a Zeek build, then change code in Zeek's Prometheus submodule, then run make, the build does nothing, because it no longer is aware of the Prometheus sources. I need to remember to go back to configuring again. I understand that this is part of the design, but I'm still wondering whether we could somehow beef up ZeekBundle.cmake so it deals with this better.

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 .so. The two .a's Prometheus builds are 350KB each, which strikes me as acceptable.

Am I roughly on track here? I'd really value your inputs.

@Neverlord
Copy link
Member Author

* The build of Prometheus currently happens during Broker's configuration phase, not Zeek's. Is that just coincidence because we configure Broker first? Broker's choice of Prometheus must not dictate Zeek's, because in the pretty near future we're going to regularly have builds (however experimental) that don't use Broker.
* It would be nice if during Zeek's own configuration stage there were some output indicating which Prometheus version it's choosing for the build. Maybe I'm missing it, but I'm not seeing any?

For me, it is using the prometheus-cpp from Zeek. Here's my output when running the configure step:

...
-- Configuring bundled project: prometheus-cpp
-- Building bundled project: prometheus-cpp as Release
-- Installing bundled project: prometheus-cpp as Release
-- Performing Test BROKER_HAS_STD_FILESYSTEM
-- Performing Test BROKER_HAS_STD_FILESYSTEM - Success
...

We have the Broker setup immediately after prometheus-cpp, so the Broker output simply happens to come next. You can confirm it's doing that before running Broker CMakeLists.txt by adding a print statement in between:

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 BROKER_HAS_STD_FILESYSTEM.

* When I kick off a debug build, Prometheus still does a Release build. Is that intentional? How would I control how it's built?

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.

* It'd be good to have a Zeek-level test that verifies that it sees Broker telemetry.

I could add a btest for that.

* At least according to our configure script, we can currently do `./configure --with-broker=...`  — but when I try that, it's ignored and the bundled Broker continues to be used. It's possible that this was broken before, I'm not sure.

Looking at the script, it'll set BROKER_ROOT_DIR. The correct variable in CMake is Broker_ROOT. So this must have been broken for quite some time now.

... The complexity comes from Broker's approach to choosing Prometheus, and the general desire to encapsulate its configuration & build as a third party...

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.

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 .so. The two .a's Prometheus builds are 350KB each, which strikes me as acceptable.

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.

@ckreibich
Copy link
Member

ckreibich commented Jul 9, 2024

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.

@ckreibich
Copy link
Member

ckreibich commented Jul 10, 2024

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 ZeekBundle, and Zeek unchanged except passing the registry into Broker.

Btw good news, I can confirm it works (with either version) — I see Broker's telemetry when scraping. 👍

@Neverlord
Copy link
Member Author

Neverlord commented Jul 10, 2024

(...) see here: #3822 (...)

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 ZeekBundle script, I'd rather ditch it entirely and look for a different solution. My original assumptions were that we don't want to touch the upstream library and that we don't want to install its targets.

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 .a file somewhere into the Zeek prefix to keep CMake happy (which the ZeekBundle tried to avoid). Would you be more comfortable with that?

@ckreibich
Copy link
Member

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.

@ckreibich
Copy link
Member

I'm closing this as it was superseded by #3822 — here too: thanks Dominik!

@ckreibich ckreibich closed this Jul 11, 2024
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.

2 participants