Telemetry Rework #3701
Telemetry Rework #3701
Conversation
timwoj
force-pushed
the
topic/timw/prometheus-cpp-3
branch
from
13a804c
to
6eadbd5
Compare
|
The docs around telemetry need some rewriting to go with this PR. |
timwoj
force-pushed
the
topic/timw/prometheus-cpp-3
branch
2 times, most recently
from
1fa97fd
to
683c882
Compare
testing/btest/scripts/policy/frameworks/telemetry/prometheus.zeek
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
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.
timwoj
force-pushed
the
topic/timw/prometheus-cpp-3
branch
2 times, most recently
from
516a228
to
43ac602
Compare
There was a problem hiding this comment.
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.)
| labels: vector of string &default=vector(); | ||
|
|
||
| ## Whether the metric represents something that is accumulating. | ||
| ## Defaults to ``T`` for counters and ``F`` for gauges and |
There was a problem hiding this comment.
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
force-pushed
the
topic/timw/prometheus-cpp-3
branch
2 times, most recently
from
21e61e9
to
23b997c
Compare
|
@ckreibich I just added two commits that change everything to doubles and removes the |
|
After thinking about the removal of the |
timwoj
force-pushed
the
topic/timw/prometheus-cpp-3
branch
from
f3271a9
to
7be0b5e
Compare
|
The os-perf numbers look pretty good:
|
timwoj
force-pushed
the
topic/timw/prometheus-cpp-3
branch
from
ccdad41
to
7be0b5e
Compare
There was a problem hiding this comment.
Good stuff Tim. I think we're basically there — I'm just leaving this at "request changes" because of my question around remaining int_ and dbl_ opaques. Let me know if I'm missing anything there.
As for all the shared pointers, they seem okay to me. It's a good fit for how they're used elsewhere in the core, and not needing to think about ownership there is the main motivator, right? I could see us moving to shortcuts later, like CounterPtr instead of shared_ptr<Counter>, if we find it too tedious.
I looked at the script-layer APIs again and I think the focus on doubles there is the right call. For APIs that update existing metrics, we already only offered doubles, so I think it's cleaner to also only offer those as doubles in the corresponding records. The value / count_value mix in the old API strikes me as clunky now.
I think any remaining deeper issues we'll really only find by experimenting with the framework.
…he service discovery endpoint
timwoj
force-pushed
the
topic/timw/prometheus-cpp-3
branch
from
0dbfc06
to
fc3ff40
Compare
timwoj
force-pushed
the
topic/timw/prometheus-cpp-3
branch
from
fc3ff40
to
52e6314
Compare
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.