-
Notifications
You must be signed in to change notification settings - Fork 40
Add profiles, move puppetdb metric defaults (part 2) #62
Conversation
375b898
to
e7a0478
Compare
302e69a
to
e41c942
Compare
409baf9
to
3547542
Compare
@suckatrash I'll try this out today or tomorrow. The only thing I noticed while doing an initial review is that the rename of the profile will be a breaking change. We should add an |
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 looks pretty awesome! A few questions in the review.
Initial impression based on reading comments above (code review coming next): this is a major change and we should document everything but I don't think we should go to any extremes on adding code for compatibility. Instead, I think this should be a major version number bump in the module and there should be a section near the top of the readme that explains changes a user will need to make to move from version A.x.x to B.0.0. WRT the suggestion above on adding a manifest in place of the old one that includes the new one, I am half in favor of this: I think the placeholder for the old one should be added but, instead of including the new manifest it should include a |
@genebean I think there's some confusion about the intent of this PR. The new profiles allow an end user to configure and run telegraf on the PE nodes, but everything is backwards-compatible with what the module did before; running it on a stand alone grafana server where telegraf runs and collects all the metrics. I think this use case is important for folks that don't necessarily have a lot of experience with metrics / monitoring as a way to "test the waters" without installing the tools all over their infrastructure. That's why I left the hostname parameters (and didn't use localhost) in the places that you commented on. Autofilling the master / puppetdb lists is something I've wanted to add for a while and I'm going to try and get that into this PR. I also like the idea of a hard fail in the deprecated class with some conspicuous documentation, and yeah I think this should be a major version bump to discourage Yolo! upgrades. |
Ahh, thanks for the clarification @suckatrash. |
@suckatrash feel free to "resolve" the comments related to localhost |
be9f57c
to
3e486f2
Compare
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.
LGTM
c0ee905
to
9d5e4b5
Compare
Until this change, the module has only allowed for running telegraf / grafana / influxdb on a single host.
In order to make this module friendlier to organizations that are already running grafana / influxdb, and only want to add Puppet metrics but lack the telegraf configuration on those hosts, I'm adding a set of profiles that can be added to the master / compilers / puppetdb so that those resources can be added independently.
Renamed
puppet_metrics_dashboard::profile::postgres
topuppet_metrics_dashboard::profile::master::postgres_access
to add clarity about whichprofile configures access and which collects metrics.
Moved all the default puppetdb metrics to params.pp so that they can be picked up by any class.
Adds puppet-telegraf as a dependency
Updated Readme to reflect that telegraf is the default and recommended collection method.
Removed the
puppet_metrics_dashboard::telegraf::install
class since the telegraf module handles that now.