Add Statsd Exporter, Mysqld Exporter, make exporters generic #27

lswith · 2016-11-17T04:26:23Z

Add a statsd exporter to the mix

oliver006 · 2016-12-09T19:20:22Z

@brutus333 - any chance this will be merged? I'd love to use this module to install the statsd_exporter and this would come in super handy.

lswith · 2016-12-12T06:33:59Z

I actually need to update this quite a bit. I've updated everything to use a daemon resource. It allows for every prometheus exporter to be added easily.

brutus333 · 2016-12-12T11:29:18Z

In this case, I'll wait a bit for your changes. Did you generalize the exporter support?

lswith · 2016-12-12T11:30:41Z

I got close but each exporter is still basically a copy paste. Take a look at my repo and see what you think

…

On Mon., 12 Dec. 2016 at 10:29 pm brutus333 ***@***.***> wrote: In this case, I'll wait a bit for your changes. Did you generalize the exporter support? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#27 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA-K-850AWUYpPVwOMI3jU0GL-pFwnPaks5rHTAOgaJpZM4K01JZ> .

brutus333 · 2016-12-12T11:56:10Z

It is looking very good at first glance but I need to test it a bit; even if each exporter needs code duplication, we can get rid of too many templates issue. One idea to go even further would be to have a defined type for exporters and applying individual values for each type of exported but the hard thing is to add custom parameters for exporters (e.g. my.cnf for mysql exporter). Something similar with keyword arguments in Python would help.
Thank you so much for your contribution, please redo the pull request with the newest additions.

oliver006 · 2016-12-12T17:35:18Z

Thanks guys, this all sounds great, especially being able to add more exporters in a more modular way.
I added the memcached exporter in my repo and will add the redis exporter soon. Once this here is merged I'll open a PR.

oliver006 · 2016-12-13T05:19:24Z

manifests/statsd_exporter.pp

+    default => undef,
+  }
+
+  $extra_statsd_maps = hiera_array('prometheus::statsd_exporter::statsd_maps')


can you add [] as the default?

oliver006 · 2016-12-13T05:19:40Z

templates/statsd_mapping.conf.erb

@@ -0,0 +1,8 @@
+<% @real_statsd_maps.each do |metric| -%>


Can you change this to real_statsd_maps.sort.each ?
Old Ruby versions have a bug where the order is not deterministic and it triggers the service restart on each run even if the data didn't change.

brutus333

Please add a comment in the README file regarding the breaking change for the alertmanager: since the name of the alertmanager service is changing from alert_manager, the user should be warned.
One solution is to stop and erase the alert_manager service before upgrading.

brutus333

Please add changes to daemon.pp to comply with #30
They are right about the location of systemd files.
Thank you

lswith · 2016-12-14T04:12:54Z

It could be hard to delete the old alertmanager completely, however stopping it should at least happen so that they don't clash.

brutus333

Not sure why you deleted these parameters. They are used in manifests.

lswith · 2016-12-14T12:50:23Z

It was a bad merge. They already exist further up

…

On Wed., 14 Dec. 2016 at 11:49 pm brutus333 ***@***.***> wrote: ***@***.**** requested changes on this pull request. Not sure why you deleted this parameters. They are used in manifests. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#27 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA-K-4TWM8c3YEIKpOPPUio_i3LW3huTks5rH-XggaJpZM4K01JZ> .

oliver006 · 2016-12-14T19:51:42Z

templates/statsd_mapping.conf.erb

+<% @real_statsd_maps.sort.each do |metric| -%>
+<%= metric['map'] %>
+name="<%= metric['name'] %>"
+<% metric['labels'].each do |key, value| -%>


Sorry, forgot to mention to also add a sort here as well.

brutus333 · 2016-12-14T20:19:58Z

I found two additional issues:

this code from alertmanager class vanished unfortunately in daemon implemnetation:
if $::prometheus::alert_manager::storage_path { file { $::prometheus::alert_manager::storage_path: ensure => 'directory', owner => $::prometheus::alert_manager::user, group => $::prometheus::alert_manager::group, mode => '0755', }
Due to this issue, the alert_manager service will not start.
$user and $group parameters are duplicated in the daemon defined type definition.

Please fix; I tested with these changes and it looks fine.

joshuaspence · 2016-12-15T23:20:59Z

Gemfile.lock

@@ -0,0 +1,61 @@
+GEM


I don't think you should commit this.

You should add Gemfile.lock to .gitignore

joshuaspence · 2016-12-18T23:45:27Z

files/statsd_mapping.conf

@@ -0,0 +1,12 @@
+test.dispatcher.*.*.*


What's the significance of this configuration?

It's the default supplied by the statsd_exporter

joshuaspence · 2016-12-19T00:05:27Z

manifests/alertmanager.pp

@@ -0,0 +1,242 @@
+# Class: prometheus::mysqld_exporter


What's the reason for renaming prometheus::alert_manager to prometheus::alertmanager? This is a breaking change.

the name of the binary and package is alertmanager not alert_manager. This was to keep consistency and allows for easier use of the daemon.pp.

joshuaspence · 2016-12-19T00:09:50Z

templates/statsd_mapping.conf.erb

@@ -0,0 +1,8 @@
+<% @real_statsd_maps.sort_by!{ |metric| metric[:name] }.each do |metric| -%>


I think that you can just use sort_by instead of sort_by

I don't understand.

joshuaspence · 2016-12-19T00:10:41Z

templates/statsd_mapping.conf.erb

+<% metric['labels'].sort.each do |key, value| -%>
+<%= key %>="<%= value %>"
+<% end -%>
+


I think that this blank line is redundant given that the previous line ends with -%>

joshuaspence · 2016-12-19T00:38:59Z

templates/daemon.sysv.erb

 #
-#       Daemonize the prometheus node exporter
+#       Daemonize the prometheus <%= @name %>


I think this should be "Prometheus <%= @name %> exporter"

joshuaspence · 2016-12-19T00:39:45Z

templates/daemon.sysv.erb

-# pidfile: /var/run/node_exporter/pidfile
+# description: Prometheus Exporter <%= @name %>
+# processname: <%= @name %>
+# pidfile: /var/run/<%= @name %>/pidfile


Should we change this to /var/run/<%= @name %>/<%= @name %>.pid for consistency with Upstart?

joshuaspence · 2016-12-19T00:40:27Z

templates/daemon.sysv.erb

-DAEMON=<%= scope.lookupvar('prometheus::node_exporter::bin_dir') %>/node_exporter
-PID_FILE=/var/run/node_exporter/node_exporter.pid
-LOG_FILE=/var/log/node_exporter
+DAEMON=<%= @bin_dir %>/<%= @name %>


This <%= @bin_dir %>/<%= @name %> logic is repeated in different template files. I think that you should define this value as a Puppet variable instead.

joshuaspence · 2016-12-19T00:42:16Z

templates/daemon.sysv.erb


-[ -e /etc/sysconfig/node_exporter ] && . /etc/sysconfig/node_exporter
+[ -e /etc/sysconfig/<%= @name %> ] && . /etc/sysconfig/<%= @name %>


I think /etc/sysconfig is a RedHat thing? This should read from /etc/default for Debian systems.

joshuaspence · 2016-12-19T00:43:33Z

templates/daemon.systemd.erb

+[Service]
+User=<%= @user %>
+Group=<%= @group %>
+ExecStart=<%= @bin_dir %>/<%= @name %> <%= @options %>


Instead, you should read the options from the defaults file. Like this:

EnvironmentFile=/etc/default/<%= @name %> ExecStart=<%= @bin_dir %>/<%= @name %> $OPTIONS

brutus333

The daemon.systemd template does not work since Shellwords.escape does not return new line separated strings.
Instead, I tested the following form:
+ExecStart=<%= @bin_dir %>/<%= @name %><% for option in Shellwords.split(@options) %>
+<%= option -%>
+<% end %>
+
The last newline is mandatory.

…daemon.pp; fixed duplicate definition of in alertmanager.pp

puppet-lint: fix arrow_on_right_operand_line

…li#27) * add a statsd_exporter * update templates * :update to use a template * fix bug * migrate to using generic service configurations * update node_exporter * update to just daemon * move node_exporter to use a generic daemon * move the statsd_exporter and alert_manager to generic daemon install * update to make the daemon a resource * update * add mysqld_exporter * update default params * update to use different users * update to use newer version of node_exporter fix mysqld_exporter my.cnf location * update systemd daemon * update to use alertmanager instead of alert_manager * update template filename * update version so that alertmanager breaking changes are known * update from review * update systemd files * update readme * the service should be restarted when the configuration is updated * fix bug in params * fix sorting of an array of hashes * add a lockfile for Gems * update documentation and sort params alphabetically * add some tests * add .gitignore file * remove Gemfile.lock * remove fixture modules * fix comments * shellescape options variable * update statsd template * Fixed daemon.systemd template; fixed duplicate definitions of and in daemon.pp; fixed duplicate definition of in alertmanager.pp * Fixed daemon upstart script * Changed statsd mapping code for ruby 1.8.7 compatibility

puppet-lint: fix arrow_on_right_operand_line

lswith changed the title ~~Add Statsd exporter~~ Add Statsd Exporter, Mysqld Exporter, make exporters generic Dec 13, 2016

lswith force-pushed the statsd_exporter branch from 7d97dbf to a28effc Compare December 13, 2016 04:48

oliver006 reviewed Dec 13, 2016

View reviewed changes

brutus333 requested changes Dec 13, 2016

View reviewed changes

This was referenced Dec 13, 2016

node-exporter have a 'v' in the release name since 0.13.0 #29

Merged

move systemd files to /etc #30

Closed

Feature add mysqld exporter #8

Closed

brutus333 requested changes Dec 14, 2016

View reviewed changes

oliver006 reviewed Dec 14, 2016

View reviewed changes

lswith added 10 commits December 19, 2016 11:24

add a statsd_exporter

adb60e8

update templates

f998195

:update to use a template

52c2aeb

fix bug

56095b7

migrate to using generic service configurations

2e1d46b

update node_exporter

58ec7bc

update to just daemon

d5f9493

move node_exporter to use a generic daemon

0fb70ef

move the statsd_exporter and alert_manager to generic daemon install

d46d8a0

update to make the daemon a resource

49bedd3

lswith added 11 commits December 19, 2016 11:24

update template filename

02855d5

update version so that alertmanager breaking changes are known

60b2176

update from review

037ddc7

update systemd files

72f1c7d

update readme

bc7aa8c

the service should be restarted when the configuration is updated

603fe3e

fix bug in params

f35167c

fix sorting of an array of hashes

cc4299a

add a lockfile for Gems

2dffd1f

update documentation and sort params alphabetically

7fcf4c1

add some tests

19a125c

lswith force-pushed the statsd_exporter branch from 6203ea5 to 19a125c Compare December 19, 2016 00:27

joshuaspence reviewed Dec 19, 2016

View reviewed changes

lswith added 6 commits December 19, 2016 11:58

add .gitignore file

25018d3

remove Gemfile.lock

b08c8a1

remove fixture modules

6593e20

fix comments

0c54101

shellescape options variable

974ba57

update statsd template

37eb8f5

brutus333 requested changes Dec 20, 2016

View reviewed changes

root and others added 3 commits December 20, 2016 13:04

Fixed daemon.systemd template; fixed duplicate definitions of and in …

cbefcf1

…daemon.pp; fixed duplicate definition of in alertmanager.pp

Fixed daemon upstart script

dc84285

Changed statsd mapping code for ruby 1.8.7 compatibility

86bd27f

brutus333 merged commit c941249 into voxpupuli:master Dec 27, 2016

brutus333 mentioned this pull request Dec 27, 2016

Support for Prometheus PushGateway #19

Closed

roidelapluie pushed a commit that referenced this pull request Jun 21, 2017

Merge pull request #27 from bastelfreak/puppet_lint

0148d7d

puppet-lint: fix arrow_on_right_operand_line

Rovanion pushed a commit to Rovanion/puppet-prometheus that referenced this pull request May 5, 2021

Merge pull request voxpupuli#27 from bastelfreak/puppet_lint

3b1ed94

puppet-lint: fix arrow_on_right_operand_line

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Statsd Exporter, Mysqld Exporter, make exporters generic #27

Add Statsd Exporter, Mysqld Exporter, make exporters generic #27

lswith commented Nov 17, 2016

oliver006 commented Dec 9, 2016

lswith commented Dec 12, 2016

brutus333 commented Dec 12, 2016

lswith commented Dec 12, 2016 via email

brutus333 commented Dec 12, 2016

oliver006 commented Dec 12, 2016

oliver006 Dec 13, 2016

oliver006 Dec 13, 2016

brutus333 left a comment

brutus333 left a comment

lswith commented Dec 14, 2016

brutus333 left a comment •

edited

Loading

lswith commented Dec 14, 2016 via email

oliver006 Dec 14, 2016

brutus333 commented Dec 14, 2016

joshuaspence Dec 15, 2016

joshuaspence Dec 18, 2016

joshuaspence Dec 18, 2016

lswith Dec 19, 2016

joshuaspence Dec 19, 2016

lswith Dec 19, 2016

joshuaspence Dec 19, 2016

lswith Dec 19, 2016

joshuaspence Dec 19, 2016

joshuaspence Dec 19, 2016

joshuaspence Dec 19, 2016

joshuaspence Dec 19, 2016

joshuaspence Dec 19, 2016

joshuaspence Dec 19, 2016

brutus333 left a comment

		@@ -0,0 +1,8 @@
		<% @real_statsd_maps.sort_by!{ \|metric\| metric[:name] }.each do \|metric\| -%>


		[ -e /etc/sysconfig/node_exporter ] && . /etc/sysconfig/node_exporter
		[ -e /etc/sysconfig/<%= @name %> ] && . /etc/sysconfig/<%= @name %>

Add Statsd Exporter, Mysqld Exporter, make exporters generic #27

Add Statsd Exporter, Mysqld Exporter, make exporters generic #27

Conversation

lswith commented Nov 17, 2016

oliver006 commented Dec 9, 2016

lswith commented Dec 12, 2016

brutus333 commented Dec 12, 2016

lswith commented Dec 12, 2016 via email

brutus333 commented Dec 12, 2016

oliver006 commented Dec 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brutus333 left a comment

Choose a reason for hiding this comment

brutus333 left a comment

Choose a reason for hiding this comment

lswith commented Dec 14, 2016

brutus333 left a comment • edited Loading

Choose a reason for hiding this comment

lswith commented Dec 14, 2016 via email

Choose a reason for hiding this comment

brutus333 commented Dec 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brutus333 left a comment

Choose a reason for hiding this comment

brutus333 left a comment •

edited

Loading