Skip to content
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

Added reference doc for Occurrences and Check_Dependencies #498

Merged
merged 21 commits into from Jul 24, 2018

Conversation

Blue0ctober
Copy link
Contributor

@Blue0ctober Blue0ctober commented Jun 14, 2018

Description

Adding references in core docs for built-in filters:

  1. occurrences
  2. check_dependencies

Closes #310

Motivation and Context

We have built-in filters, extensions included in official Sensu Core packages, which should be mentioned along site native or configuration-defined filters.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • New doc/guide
  • Fixing errata
  • Update (Add missing or refresh existing content)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • All tests have passed.

@Blue0ctober Blue0ctober requested a review from a team as a code owner June 14, 2018 19:24
@amdprophet amdprophet added the in progress Currently being worked label Jun 14, 2018
@Blue0ctober Blue0ctober added ready for review PR is ready to review and removed in progress Currently being worked labels Jun 14, 2018

The `check_dependencies` filter is included in every install of Sensu. This
filter can be applied to a handler using the "filter" or "filters" handler definition
attribute. The `check_dependencies` filter matches events when an even already exisits,
Copy link
Contributor

@rgeniesse rgeniesse Jun 14, 2018

Choose a reason for hiding this comment

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

events when an event instead of events when an even
and
exists instead of exisits

### Built-in Filters - check_dependencies {#built-in-filters-check-dependencies}

The `check_dependencies` filter is included in every install of Sensu. This
filter can be applied to a handler using the "filter" or "filters" handler definition
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm leery of putting "filter" in here. Best practice would be for customers to use "filters".

### Built-in Filters - occurrences {#built-in-filters-occurrences}

The `occurrences` filter is included in every install of Sensu. This filter can
be applied to a handler using the "filter" or "filters" handler definition attribute.
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't notice this at first, but yeah, I'd recommend enforcing "filters"

@apaskulin
Copy link
Contributor

This looks great! I love the examples you've used, especially the progression of example for dependencies. I'd recommend:

  • breaking out "Built-in Filters" into its own section (if that makes sense to you) instead of embedding it within "Filter configuration"
  • adding the new headings to the TOC at the top of the page
  • using tables for the occurrences and refresh attributes

@asachs01
Copy link
Contributor

Sidenote, I feel like this would satisfy the #374 issue. It's not a full-blown guide, but it adds the requisite material.

}
{{< /highlight >}}

Lastly we can specify a dependency on any `mysql` check in the `mysql_nodes` subscription:
Copy link
Contributor

Choose a reason for hiding this comment

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

The subscription: prefix functionality added in sensu/sensu-extensions-check-dependencies#12 has been merged, but we haven’t cut a new release of check_dependencies extension since that time.

This means that the feature hasn’t yet shipped to customers, so we shouldn’t describe it in these versions of the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cwjohnston was this for only 0.29 or everything including 1.4?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Blue0ctober we'll need to remove references to subscription/check pairs and the subscription: prefix for all current versions. Ideally we'll ship that feature in Sensu 1.5.

{{< /highlight >}}


The `occurrences` filter uses two custom check definition attributes, `occurrences`
Copy link
Contributor

Choose a reason for hiding this comment

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

Let’s drop the adjective “custom” here. They are more or less part of the spec IMO.

Copy link
Contributor

@cwjohnston cwjohnston left a comment

Choose a reason for hiding this comment

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

I think a couple of changes are needed. Please have a look.


The `check_dependencies` filter uses a custom check definition attribute `dependencies`.
`dependencies` is defined as an array with attributes such as `checks`, Sensu client/check
pairs or subscription/check pairs.
Copy link
Contributor

Choose a reason for hiding this comment

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

@Blue0ctober lets rephrase this to remove or subscription/check pairs.

@apaskulin apaskulin requested review from apaskulin and removed request for apaskulin July 9, 2018 16:31
@asachs01
Copy link
Contributor

@Blue0ctober is this all set?

@apaskulin apaskulin changed the title Added reference doc for Occurrences and Check_Dependencies [WIP] Added reference doc for Occurrences and Check_Dependencies Jul 16, 2018
Edgar Valdes added 3 commits July 18, 2018 15:37
Copy link
Contributor

@apaskulin apaskulin left a comment

Choose a reason for hiding this comment

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

Super excited about this! Left a few comments here and in Slack. We'll also need to get an approval from @cwjohnston before GitHub will let us merge.

- [occurrences Filter Attributes](#occurrences-filter-attributes)
- [check_dependencies Filter](#check-dependencies-filter)
- [Defining Check Dependencies](#defining-check-dependencies)
- [check_dependencies Attributes](#check-dependencies-attributes)
Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend reducing this to just:

[Built-in Filters](#built-in-filters)



The `occurrences` filter uses two check definition attributes, `occurrences`
and `refresh`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this sentence is a duplicate of line 327

The `dependencies` attribute should define an array containing names of checks or
client/check pairs.

This example showcases a check configured to depend on any check called `mysql`:
Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend changing "showcases" to "shows"


The occurrences filter lets you configure the number of occurrences and the refresh interval at the check level using two check definition attributes: `occurrences` and `refresh`.

Here's an example of a check definition that passes events to the `email` handler starting with the third occurrence every 60 minutes:
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs review

}
}
}
{{< /highlight >}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understand this behavior. Which event is being filtered: mysql or web_application_api?

Copy link
Contributor

Choose a reason for hiding this comment

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

If mysql check for this client is in a non-OK state, events from this client for web_application_api check from will be filtered.

@apaskulin apaskulin changed the title [WIP] Added reference doc for Occurrences and Check_Dependencies Added reference doc for Occurrences and Check_Dependencies Jul 24, 2018

The occurrences filter lets you configure the number of occurrences and the refresh interval at the check level using two check definition attributes: `occurrences` and `refresh`.

Here's an example of a check definition that passes events to the `email` handler starting with the third occurrence every 60 minutes:
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this be starting with the second occurrence every 60 minutes? The JSON example has occurrences set to 2, so I believe it would be handled after 2 occurrences. I can test this if we aren't sure.

Copy link
Contributor

@apaskulin apaskulin Jul 24, 2018

Choose a reason for hiding this comment

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

Thanks for clarifying! That was exactly what I was wondering


The check dependencies filter is included in every install of Sensu.
This filter can be applied to a handler using the `filters` handler definition attribute.
The check dependencies filter lets you reduce notification noise by matching events when an event already exists, thereby only notifying for the root cause of a given failure.
Copy link
Contributor

Choose a reason for hiding this comment

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

This part I think needs changed: by matching events when an event already exists.
The point of dependencies is to get to the root cause quicker. So if a tomcat process isn't running, a check that tries to get to the API for an application inside of tomcat would not work. API depends on tomcat, if tomcat down only handle that event.

Maybe something like by looking to see if an event higher up in the dependency tree already exists

@apaskulin apaskulin changed the title Added reference doc for Occurrences and Check_Dependencies [WIP] Added reference doc for Occurrences and Check_Dependencies Jul 24, 2018
@apaskulin apaskulin changed the title [WIP] Added reference doc for Occurrences and Check_Dependencies Added reference doc for Occurrences and Check_Dependencies Jul 24, 2018
Copy link
Contributor

@rgeniesse rgeniesse left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@apaskulin apaskulin merged commit bbc30d1 into master Jul 24, 2018
@apaskulin apaskulin deleted the update/built-in-filters branch July 24, 2018 22:22
@Blue0ctober Blue0ctober removed the ready for review PR is ready to review label Jul 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Filters reference doc should describe built-in filters
6 participants