Skip to content
This repository has been archived by the owner on Jun 1, 2023. It is now read-only.

Add profiles, move puppetdb metric defaults (part 2) #62

Merged
merged 4 commits into from
Jul 8, 2019

Conversation

suckatrash
Copy link
Contributor

@suckatrash suckatrash commented Jun 12, 2019

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 to puppet_metrics_dashboard::profile::master::postgres_access to add clarity about which
profile 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.

@suckatrash suckatrash force-pushed the add_profiles branch 3 times, most recently from 375b898 to e7a0478 Compare June 14, 2019 21:40
@suckatrash suckatrash force-pushed the add_profiles branch 5 times, most recently from 302e69a to e41c942 Compare June 27, 2019 22:23
@suckatrash suckatrash force-pushed the add_profiles branch 6 times, most recently from 409baf9 to 3547542 Compare June 28, 2019 21:11
@jarretlavallee
Copy link
Contributor

@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 puppet_metrics_dashboard::profile::postgres class that emits a deprecation warning. It could include puppet_metrics_dashboard::profile::postgres_access automatically if you think that would be appropriate.

Copy link
Contributor

@jarretlavallee jarretlavallee left a 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.

manifests/telegraf/install.pp Outdated Show resolved Hide resolved
manifests/init.pp Show resolved Hide resolved
manifests/init.pp Show resolved Hide resolved
manifests/telegraf/config.pp Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@genebean
Copy link
Contributor

genebean commented Jul 2, 2019

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 fail('This class has been replaced by "puppet_metrics_dashboard::profile::postgres_access" Please update your code to include that class instead of this one') or similar. Basically, I am in favor of a fail message that directs the user on how to fix their code as opposed to a depreciation warning they may well never see.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
manifests/profile/compiler.pp Show resolved Hide resolved
manifests/profile/puppetdb.pp Show resolved Hide resolved
manifests/profile/puppetdb.pp Show resolved Hide resolved
manifests/profile/compiler.pp Show resolved Hide resolved
manifests/profile/compiler.pp Show resolved Hide resolved
manifests/telegraf/config.pp Outdated Show resolved Hide resolved
@suckatrash
Copy link
Contributor Author

@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.

@genebean
Copy link
Contributor

genebean commented Jul 2, 2019

Ahh, thanks for the clarification @suckatrash.

@genebean
Copy link
Contributor

genebean commented Jul 2, 2019

@suckatrash feel free to "resolve" the comments related to localhost

@suckatrash suckatrash force-pushed the add_profiles branch 2 times, most recently from be9f57c to 3e486f2 Compare July 3, 2019 18:04
Copy link
Contributor

@genebean genebean left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@suckatrash suckatrash force-pushed the add_profiles branch 6 times, most recently from c0ee905 to 9d5e4b5 Compare July 8, 2019 17:32
@suckatrash suckatrash merged commit 34fda2c into puppetlabs:master Jul 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants