Telemetry Rework

[timwoj]

This PR represents all of the work to pull the telemetry code out of broker and into a module inside of Zeek itself. The major change here is usage of the prometheus-cpp library for all of the storage and coordination of families and instruments. It also moves to a setup where we provide a Prometheus Service Discovery endpoint for automatic configuration of all of the nodes in Prometheus, removing the internal aggregation of data onto the manager node for Prometheus to scrape.

[timwoj]

The docs around telemetry need some rewriting to go with this PR.

[bbannier]

I am not a fan of coupling a specific port value 0/unknown to disabling metrics functionality.

With this patch users are able to query an endpoint for the actually used port which opens up the door to let the OS assign ports which could simplify configuration; typically port 0 is used for that, mirroring the behavior of bind.

It seems that the underlying endpoint impl can both parse addresses with port 0 as well as pass that through to the OS for automatic port assignment so IMO it would be unfortunate to completely close the door on even allowing that by this API choice.

Would you be open to instead introducing a dedicated flag for to toggle metrics functionality, e.g., --enable-metrics or --disable-metrics?

[bbannier]

An intermediate solution could be to sharpen the interpretation of "port 0" when parsing the config to only interpret 0/unknown (or maybe any port on an unknown protocol) as disabling,

zeek/src/telemetry/Manager.cc

Line 34 in 3c811ec

if ( metrics_port->Port() != 0 )

With that it would be impossible to disable metrics via the ZEEK_METRICS_PORT env var though since it never contains a protocol, only a port number.

[timwoj]

It seems that the underlying endpoint impl can both parse addresses with port 0 as well as pass that through to the OS for automatic port assignment so IMO it would be unfortunate to completely close the door on even allowing that by this API choice.

We don't notify the user in any way what port number is being used by the telemetry manager, since we expect them to set it themselves as part of the configuration. If CivetWeb picks its own port, the user won't know what that port is, nor will the services.json endpoint know what any of the ports are. I'm inclined to not allow it to be configured like that.

Would you be open to instead introducing a dedicated flag for to toggle metrics functionality, e.g., --enable-metrics or --disable-metrics?

How about a script-level setting that overrides any other ports you set? That would cover cluster mode.

With that it would be impossible to disable metrics via the ZEEK_METRICS_PORT env var though since it never contains a protocol, only a port number.

In standalone mode, not setting ZEEK_METRICS_PORT and not setting Telemetry::metrics_port already disables creating the Prometheus server.

[ckreibich]

Much of this is just moving things over from how they worked in Broker-land ... but I agree that we should revisit some of it. See my comment above in options.zeek about whether we even need the ZEEK_METRICS_PORT environment variable?

In terms of enabling, I agree that we shouldn't tie lots of functionality to the value of a port. But here it's really just whether there's a listening port for scraping or not — not the greater telemetry machinery. It makes sense to keep telemetry itself functional regardless of this value since there's also a telemetry log, plus the functionality is worthwhile just for the script layer too.

[timwoj]

I ended up removing all of the environment variables, in favor of controlling everything from script-land instead.

[ckreibich]

Good stuff Tim! I really like how this shifts the code — it actually hasn't grown anywhere near as much as I suspected. I'm still not through all of this though, sigh — I have not yet reviewed the test cases, so I'll follow up on these.

My comments mainly flag things where I see an opportunity in this rework to simplify some things from the Broker era that may not hold any more.

I'll review the docs separately, but I noticed that it still mentions Broker stuff in a few places. We'll need to make a broader edit pass on those. It would also be helpful to put a brief recap of the basic concepts (families, instruments, etc) somewhere in the code, or perhaps the development section of the docs, since things like families or instruments are all mysterious if one doesn't know Prometheus.

[ckreibich]

I'm now all the way through Tim, great stuff — I like that we're removing more things now.

I will tinker with this some more — I'm tempted to add a test case for it over in zeek-testing-cluster since it would nicely complement the approach here in the btests. I have a PR coming up to simplify a few things in the supervisor where we'd otherwise need to reflect the metrics port in a few data structures. (This was this comment on Slack a while back.)

[ckreibich]

Yeah. I just followed the code path for is_total and it now really is only used to add the suffix _total to the Prometheus metrics name, right? Since we have the metric_type below, and we generally know whether we're operating on a counter, gauge, or histogram elsewhere in the code, it seems we can always just use _total automatically for counters. (This is a Prometheus requirement, right?) It seems safe to me to deprecate this field (or just remove it outright, given the next point...).

As to double vs count ... even in the prior code we only supported doubles in various instrument-adjusting script-level APIs, right? It seems to me the difference is really a detail. It could matter for example for meaningfully plotting the metrics based on that typing info, for small values (it doesn't make sense to say "3.5 packets" or some such). But that looks pretty irrelevant: for large values the visual difference is negligible, and if one knows which metrics one is dealing with, one already knows whether they deal in doubles or counts.

Long story short: it looks safe to me to reduce this to just having all instruments operating on doubles. If we do that, it brings up the question of how much we let it affect the C++ design around BaseType etc. I could see keeping that as-is, in case we make other customizations in the future?

[timwoj]

@ckreibich I just added two commits that change everything to doubles and removes the is_sum argument everywhere.