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

Use match.Matcher for checking heartbeat response bodies #5981 #6539

Merged
merged 5 commits into from
Apr 10, 2018

Conversation

Crazybus
Copy link
Contributor

The current behaviour of check.response.body is to match the entire response body. This works great for simple examples however doesn't work so great when you want to find a small string in a large body.

As discussed in #5981 I have modified the check.response.body parameter to expect a regex pattern (match.Matcher) instead of the entire body string to match.

The awesome thing is this remained fully backwards compatible with the old body check method without any config changes even when using the json example in the docs :)

Another nice enhancement that I added was the use of subtests. I originally copied some tests from another beat and was disappointed to find out that it would stop as soon as a test case failed and not run all of them. Subtests allows you to run all tests using the table driven pattern with the output and behaviour of writing multiple tests.

❯ go test github.com/elastic/beats/heartbeat/monitors/active/http -v
=== RUN   TestCheckBody
=== RUN   TestCheckBody/Single_regex_that_matches
=== RUN   TestCheckBody/Regex_matching_json_example
=== RUN   TestCheckBody/Regex_matching_first_line_of_multiline_body_string
=== RUN   TestCheckBody/Regex_matching_lastline_of_multiline_body_string
=== RUN   TestCheckBody/Regex_matching_multiple_lines_of_multiline_body_string
=== RUN   TestCheckBody/Regex_not_matching_multiple_lines_of_multiline_body_string
=== RUN   TestCheckBody/Single_regex_that_doesn't_match
=== RUN   TestCheckBody/Multiple_regex_match_where_at_least_one_must_match
=== RUN   TestCheckBody/Multiple_regex_match_where_none_of_the_patterns_match
--- PASS: TestCheckBody (0.00s)
    --- PASS: TestCheckBody/Single_regex_that_matches (0.00s)
    --- PASS: TestCheckBody/Regex_matching_json_example (0.00s)
    --- PASS: TestCheckBody/Regex_matching_first_line_of_multiline_body_string (0.00s)
    --- PASS: TestCheckBody/Regex_matching_lastline_of_multiline_body_string (0.00s)
    --- PASS: TestCheckBody/Regex_matching_multiple_lines_of_multiline_body_string (0.00s)
    --- PASS: TestCheckBody/Regex_not_matching_multiple_lines_of_multiline_body_string (0.00s)
    --- PASS: TestCheckBody/Single_regex_that_doesn't_match (0.00s)
    --- PASS: TestCheckBody/Multiple_regex_match_where_at_least_one_must_match (0.00s)
    --- PASS: TestCheckBody/Multiple_regex_match_where_none_of_the_patterns_match (0.00s)
=== RUN   TestSplitHostnamePort
--- PASS: TestSplitHostnamePort (0.00s)
PASS
ok  	github.com/elastic/beats/heartbeat/monitors/active/http	0.019s

Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the contribution especially for adding docs and tests. Left a minor comment. LGTM.

@urso Do you also want to have a look?

- type: http
schedule: '@every 5s'
urls: ["https://myhost:80"]
check.request:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the indentation is off here. It should be on the same level as urls for example.

Same in all the examples below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch! The current example also has the wrong indentation too (which I copy pasted from)
image. So I'll fix up the original too.

@ruflin ruflin requested a review from urso March 13, 2018 14:39
if !bytes.Equal(body, content) {
return errBodyMismatch
for _, m := range body {
if m.MatchString(string(content)) {
Copy link

Choose a reason for hiding this comment

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

use m.Match(content), so to reduce the chance of allocating and copying all contents, just for matching.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

}

for _, test := range matchTests {
t.Run(test.description, func(t *testing.T) {
Copy link

Choose a reason for hiding this comment

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

+1


for _, test := range matchTests {
t.Run(test.description, func(t *testing.T) {
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
Copy link

Choose a reason for hiding this comment

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

Requiring an http server is somewhat overkill. Makes me wonder if we can 'simplify' the validators interface/function, so to not depend on a http response.

Especially the fact the body validator consuming the complete body is not useful if we want to be able to combine (and/or) more complicated filtering in the future.

Using interfaces one could define:

type response struct {
  Status() uint
  Body() ([]byte, error)
  ...
}

The adapter for use with http.Response could lazily read and cache the body then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I totally agree 👍

Makes me wonder if we can 'simplify' the validators interface/function, so to not depend on a http response.

The wording you used here makes this comment sound like "something we would like to improve in the future". Can you confirm that I interpreted this correctly? Otherwise I'd be happy to have a go refactoring in your suggestion but I would definitely need a bit of help since this goes beyond my basic go knowledge.

Copy link

Choose a reason for hiding this comment

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

Yeah, it's something we can do in the future. Rather have not 'polute' this PR with too many cleanups.

var matchTests = []struct {
description string
body string
patterns []match.Matcher
Copy link

Choose a reason for hiding this comment

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

as these are patterns, how about []string, so to make the table a little more readable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it isn't obvious I'm still very new to writing go :) Does this look a bit more readable now? f63257b Or is there maybe an even cleaner way to do it?

@urso
Copy link

urso commented Mar 13, 2018

Switching to match.Matcher is not exactly backwards compatible. The original string comparison did require an exact match. The match.Matcher implementation checks for a matching sub-string in the body. => users will have to use ^<contents string>$, so to have exact same behaviour and runtime complexity.

For the future I was considering more enhanced body matching, that can also support parsing json/xml, so to check a document field it's value. This would require the body validator to provide different validator types. Like:

full contents:

body.equals: <full contents string>

using matches:

body.matches: <regex>

json:

body.json:
  field: ...
  equals: ...

...

@ruflin
Copy link
Contributor

ruflin commented Mar 15, 2018

@urso I would be fine with the breaking change here as we are still in beta and can document it. WDYT? In both cases the change to the above propose structure will be breaking I think.

@Crazybus
Copy link
Contributor Author

@urso Thanks for the review! I'm still very new to writing Go so the feedback really taught me a lot!

Switching to match.Matcher is not exactly backwards compatible. The original string comparison did require an exact match. The match.Matcher implementation checks for a matching sub-string in the body. => users will have to use ^$, so to have exact same behaviour and runtime complexity.

Ahh yes you are right!

It would be easy enough to modify the regex to remain backwards compatible by requiring it to be an exact match. I don't feel strongly either way but I personally would vote for the default to be "match this string anywhere in the contents". In my experience checking the body is normally done as a "site is up and has real content" sanity check. Instead of only checking for a HTTP 200 which a lot of "website is on fire" status pages return.

However as a user I'm trying to think of a realistic scenario where this change in behaviour would actually cause issues. I can think of one example where this could bite someone but I would really hope nobody has a healthcheck endpoint like this.

  • If a user had a health check endpoint that returned ok and not ok and check.response.body: ok and both endpoints returned a 200. With the new regex style checking the body would match for both outputs.

For the future I was considering more enhanced body matching, that can also support parsing json/xml, so to check a document field it's value. This would require the body validator to provide different validator types.

I like all of these suggestions. In case you missed it I made some similar proposals in #5981 . The main one I am interested in is having a proper way to check JSON output. The main use case would be monitoring the health of our Elasticsearch clusters that are running in cloud.

check.response:
  bodyJSON:
     status: "green"

Which could of course be done with a regex too

check.response:
  body: '"status": "green"

@ruflin
Copy link
Contributor

ruflin commented Mar 16, 2018

To move forward on this I suggest we go with the breaking change but document it properly in the changelog. From there we can further discuss about the above proposal to make it even more powerful.

@urso
Copy link

urso commented Apr 6, 2018

@Crazybus I see you put this PR 'On Hold'. Personally I'm fine with the breaking change, as it's still a massive improvement over the current situation.

Given there are no critical blockers in this PR, how about merging it as is and do further (general) improvements on validation in future PRs.

@Crazybus
Copy link
Contributor Author

Crazybus commented Apr 6, 2018

@Crazybus I see you put this PR 'On Hold'.

@urso

Sorry for the confusion, my "On Hold" status is also used when I'm waiting to hear back from someone (in this case you).

Given there are no critical blockers in this PR, how about merging it as is and do further (general) improvements on validation in future PRs.

Sounds good to me 👍Is it ok for me to merge this in myself? I do have the rights (as a GitHub admin) however I'm not part of the beats team so I just wanted to check first.

@urso
Copy link

urso commented Apr 6, 2018

Normally the PR dev doesn't merge the PR.

Can you rebase on top of master and add an changelog entry? I will merge the PR once CI is green. Thanks!

Crazybus added 5 commits April 9, 2018 08:24

Verified

This commit was signed with the committer’s verified signature.
Crazybus Michael Russell

Verified

This commit was signed with the committer’s verified signature.
Crazybus Michael Russell

Verified

This commit was signed with the committer’s verified signature.
Crazybus Michael Russell

Verified

This commit was signed with the committer’s verified signature.
Crazybus Michael Russell

Verified

This commit was signed with the committer’s verified signature.
Crazybus Michael Russell
@Crazybus Crazybus force-pushed the match_all_the_matches branch from f63257b to e71f6df Compare April 9, 2018 06:37
@Crazybus
Copy link
Contributor Author

Crazybus commented Apr 9, 2018

@urso Added a changelog entry, rebased and CI is green!

@urso urso merged commit 8edea13 into elastic:master Apr 10, 2018
@urso
Copy link

urso commented Apr 10, 2018

Merged. Thank you for improving Heartbeat!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants