Open hillaryfraley opened 2 years ago
@hillaryfraley thanks for this PR, and the others you've submitted! These are fantastic improvements overall!
@jspaleta, @thoward, and myself have reviewed a few of them to understand the various different categories of changes you're proposing, and we have the following comments/feedback:
We would like to standardize on a consistent set of integration README headings. To this end, we like your suggestion to split the "Metrics & Events" into two separate H2 headings; however, we would like to avoid adding one-off H3 headings.
I'm indifferent on renaming "events" to "alerts" (I had initially proposed events because that's what DataDog uses, and because we don't know for sure if the user will configure an alert or incident pipeline, but we will always generate an event). Would it make more logical sense to put the metrics subheading before events/alerts subheading?
It looks like the H3 headings you are recommending generally fall into two categories: a) providing additional context for an existing setup step (e.g. the ### Remediation action annotation
heading in this commit), or b) general information about Sensu that may not be required to use the integration (e.g. the ### Handler templating
heading in this commit).
In the former case (additional context for existing setup steps), we'd like to include these inline along with the setup steps. In the latter case (general information) we'd like to put these in the "reference documentation" section of the README. I went ahead and made those changes for this PR in this commit as an example.
We like the change to make the metrics/alerts/incidents pipeline prompts more consistent across the integrations. 💯
We like the change to remove default pipelines (e.g. a generic "metrics" pipeline); I think this is an artifact of an idea we had to promote some best practices that we didn't follow through on, so removing them makes sense. 👍
Thank you for your many functional edits (e.g. removing errant linux/darwin/windows subscriptions from third-party service integrations such as the etcd-monitoring integration)! 🙏
Thank you for adding post install content where these were missing! 👌
In an effort to avoid intimidating looking setup steps, we like the idea of leverage the html <details>
tag where appropriate – but we're not sure if it's necessary to make this change in every single integration. For now I think we can/should make judicious use of the <details>
tag. I toyed around with using the <details>
tag for this PR in this commit as an example. Any thoughts on this front?
I think these are all the comments we had in our initial review. Let me know if you have any questions/suggestions in response to this feedback (you're more than welcome to disagree!). Thanks again for submitting these PRs!! 🙇♂️
Many thanks for these detailed comments! I'm glad that overall this is helpful and everything in your comment makes sense. Thank you (and @jspaleta and @thoward) for this feedback and for being willing to let me try my hand here. I am learning a lot.
As for events/alerts/metrics -- my thinking was that even though metrics and alerts both come from events, from the user's perspective the integrations are sending metrics and alerts. It's a little odd for me too b/c we don't talk about events that way in the docs, but the docs level of precision doesn't seem right in the Catalog either.
I put Alerts before Metrics mainly because some of the metrics lists are pretty long (I was hoping it would help people see/find the Alerts heading in those cases).
I meant for the H3 elements in the Setup section to communicate that those actions are optional (the numbered steps describe things you must have/know to use the integration or so that the installation process makes more sense…then the H3 stuff describes the customization options). However I see now that this is not obvious! It’s more elegant to include the appropriate options as you have in the Ansible commit and it makes sense to list the rest in reference documentation.
The details tag seems like a good solution for keeping the setup steps as tidy as possible while providing the information users need to customize.
Thanks again! I will update the PRs I've submitted to adhere to the updated Ansible commit.
Includes minor text revision in README, as well as these documented in the changelog: