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
write_prometheus plugin: New plugin for exposing metrics to Prometheus. #1967
Conversation
This looks promising, thanks a lot for working on that ! I'll take a moment today to rebuild the jenkins workers with microhttpd, and re-trigger the builds. Count on me for testing this out in the near future :-) |
Thanks @mfournier, I was about to ask you how to get libmicrohttpd on the build minions ;) |
Let me point out some differences to collectd_exporter to look out for:
|
There are some build issues on Precise, presumably due to an old version of libmicrohttpd:
|
Wheezy is still having issues:
Wheezy has libmicrohttpd-dev 0.9.20-1+deb7u1. |
Great success! |
8aed454
to
0686aed
Compare
I'm quite keen to get this plugin, because the collectd_exporter is a thing that adds latency and can possibly fail. Therefore a big thanks for getting this in @octo! In my test setup (>25k lines in /metrics) I get lots of errors like this:
I naively just assume this is the reason why the process becomes bigger and bigger over time (starting single digit GB, grows up to the point where there is no more free memory) and eventually gets oom killed ( There is a second thing that I think is unrelated, but I'm not sure where else to put that. And maybe it's related as well, as I've not noticed that error before playing with this plugin.
|
Hi @bs-github, thanks for your feedback! "Status 2" sounds like I've run the plugin with Valgrind, but that of course only means that the shutdown handler works as intended. I'll see if I can track down a life-leak somewhere … Best regards, |
@octo: what you describe as the missing event was received more than once is highly likely what happens here. I have some issues with such cases in my setup. |
Another thing: I'm quite sure, that collectd_exporter will stop exporting a metric when it is no longer updated as well. See here: https://github.com/prometheus/collectd_exporter/blob/master/main.go#L170-L179 |
@octo: As you suggested I added an "abort();" at the beginning of the shutdown callback. Then I've recompiled with debug enabled But the last line in the log is catching my eye. Is that a normal thing caused by the abort call?
I've not yet done the thing with the traffic generator, neither did I cut of the regular feed that I use for testing.
|
I get the following error when building against libmicrohttpd 0.9.51:
edit: it seems github now allows pushing to other peoples branches, so I did just this against https://github.com/octo/collectd/tree/ff/write_prometheus. |
@bs-github can you try running collectd under valgrind's massif tool ?
This should show the function calls which allocate memory over time, and hopefully give a hint on what's going on. Thanks ! |
results from running under valgrind --tool=massif:
|
@bs-github Could you please confirm that this is related to the write_prometheus plugin? The trace indicates that this is leaked in a location that is not changed by this PR. If the memory grows without this plugin, too, please open a separate issue and attach the entire massif output file? It ran this plugin with massif last week and also found that lots of memory is allocated in Thanks and best regards, |
@bs-github ping? |
FWIW, I noticed a leak in 5.6 too without this plugin. |
As for the PR itself, it looks good to me, nice work @octo! |
Sorry for the late reply, I've been busy with other things. But to get back on topic, as far as I remember, I've had an instance of collectd (same binary, separate config) running that only worked as a forwarder (network proxy setup). That instance had no memory issues (or just such minor ones that I've not noticed them). This observation does indeed not tell us that the issue is with this PR. In fact, at this point, I doubt it. So, I'm good with moving this issue out of this PR. @octo I do have the full |
Alright, I'll go ahead and merge this. @rubenk, @bs-github, could one of you please open a bug report against 5.6 regarding the memory leak? Birger, you can attach the massif output to the issue by drag-and-dropping it into the browser window. Thanks everybody! 🎉 |
Profiling showed that prom_write() spent 73% of its time in this function. 36% of time was spent in metric_create() and 19% was spent in metric_destroy(). This patch replaces these two calls by a stack allocation, reducing the time prom_write() spends in metric_family_get_metric() to 42%.
This function is a hotspot because it is used by bsearch() to look up metrics in a metric family. This simple (though non-obvious) change brings prom_write() complexity down from 3000 instructions/call to 2640 instructions/call, i.e. a 12% improvement.
Add switch on MHD_VERSION to support both legacy and modern MHD functions. `MHD_create_response_from_data()` is deprecated since libmicrohttpd 0.9.5 and makes the build fail since 0.9.45.
99ba7a9
to
c53c496
Compare
Rebased on a more recent master. Once the build bots are happy I'll merge. |
…_DEFAULT_STALENESS_DELTA. Fixes: write_prometheus.c:56:1: error: initializer element is not constant static cdtime_t staleness_delta = PROMETHEUS_DEFAULT_STALENESS_DELTA; ^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this. Here's some things I noticed in passing.
|
||
Port the embedded webserver should listen on. Defaults to B<9103>. | ||
|
||
=item B<StalenessDelta> I<Seconds> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this setting is going to cause weirdness. If you're going to expose timestamps, always expose timestamps for a use case like this.
This is also what you will be doing once we fix staleness handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the behavior that came out of the discussion in prometheus/collectd_exporter#24. Can you give me a reference to where this fix is being discussed or implemented? If that fix lands before collectd 5.7 is released I'm happy to remove this setting. Otherwise, we're going to add a deprecation notice when this setting is no longer needed and eventually change the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only discussed GC, not this sort of behaviour. The issue is what you've implemented here will break both now and once we finally fix staleness.
Consider that there's a metric being forwarded through a few systems, so that it's hovering around the 5m old mark when it gets to Prometheus. Every time it goes over 5m, Prometheus will see it as now. As Prometheus only allows you to append samples in order, this means the next 5 minutes of samples under 5 minutes old will be rejected and errors logged.
To avoid this you should always expose timestamps.
ssnprintf(line, sizeof(line), "%s{%s} " GAUGE_FORMAT "%s\n", fam->name, | ||
format_labels(labels, sizeof(labels), m), m->gauge->value, | ||
timestamp_ms); | ||
else /* if (fam->type == IO__PROMETHEUS__CLIENT__METRIC_TYPE__COUNTER) */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation doesn't handle Histograms or Summarys correctly. While collectd won't be exporting any, can you add a comment warning anyone looking to reuse this code elsewhere?
c_avl_iterator_destroy(iter); | ||
|
||
char server[1024]; | ||
ssnprintf(server, sizeof(server), "\n# collectd/write_prometheus %s at %s\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use the approach at https://www.robustperception.io/exposing-the-software-version-to-prometheus/ to get the version into Prometheus
|
||
for (size_t i = 0; i < m->n_label; i++) | ||
ssnprintf(labels[i], LABEL_BUFFER_SIZE, "%s=\"%s\"", m->label[i]->name, | ||
m->label[i]->value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure the values won't need escaping?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
→ #2035
* 3 labels: | ||
* [0] $plugin="$plugin_instance" => $plugin is the same within a family | ||
* [1] type="$type_instance" => "type" is a static string | ||
* [2] instance="$host" => "instance" is a static string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we presume a collectd per machine with no forwarding then there's no need to expose an instance label, as Prometheus already knows it. The user will need to remove the exported_instance
label you're adding here (prefixed due to collision handling) in metric_relabel_configs
.
If there is forwarding going on, then the docs should mention to enable honor_labels: true
. Getting Prometheus hitting all the collectds directly would be preferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As there already exists several ways to forward metrics from one collectd to another, we can expect some users will setup a central instance collecting values from a fleet of agents, and have prometheus point to this single central collectd.
I agree with you it's better to have collectd hit the "data source" directly, but I'm also aware some people have legacy machines or exotic architectures on which they won't/can't install the latest collectd, nor any prometheus exporter. These will be the typical users of forwarding I guess.
A general question, how long would you expect this code to get out to the majority of your users? Once this is fully deployed, we can deprecate the collectd_exporter. |
@brian-brazil the write_prometheus plugin will be part of collectd 5.7.0, due in roughly one month from now. I can't give a good answer about when it would be "fully deployed", but most linux/BSD distros usually have collectd releases packaged within a couple of months. I wouldn't depreciate the collectd_exporter too quickly though (cf the folks blocked with a collectd version from several years ago). |
Also, many thanks @octo ! Having a proper bridge between these 2 awesome tools is really great ! |
@octo I kept an eye on my collectd instance, and for the last 3 days the RSS stayed the same at 160MB, VSS at 1471MB, so this might have been a false alarm. I do see my rrdcached process slowly increases, so the leak might be in there. |
This new plugin embeds an HTTP server that can be scraped from Prometheus, similar to the way collectd_exporter works. The metric family names and labels are created in the same manner as collectd_exporter so this plugin can function as a drop-in replacement.