-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
JSON Checks for Heartbeat HTTP Monitors #8667
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,7 +24,12 @@ import ( | |
"net/http/httptest" | ||
"testing" | ||
|
||
"github.com/elastic/beats/libbeat/common" | ||
|
||
"github.com/stretchr/testify/require" | ||
|
||
"github.com/elastic/beats/libbeat/common/match" | ||
"github.com/elastic/beats/libbeat/conditions" | ||
) | ||
|
||
func TestCheckBody(t *testing.T) { | ||
|
@@ -125,3 +130,69 @@ func TestCheckBody(t *testing.T) { | |
}) | ||
} | ||
} | ||
|
||
func TestCheckJson(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These tests are a bit redundant given the python integration tests. However, it's a nicer dev/test cycle when using these. I wrote these first. I'm +1 to just leave them in, they could be useful in the future. |
||
fooBazEqualsBar := common.MustNewConfigFrom(map[string]interface{}{"equals": map[string]interface{}{"foo": map[string]interface{}{"baz": "bar"}}}) | ||
fooBazEqualsBarConf := &conditions.Config{} | ||
err := fooBazEqualsBar.Unpack(fooBazEqualsBarConf) | ||
require.NoError(t, err) | ||
|
||
fooBazEqualsBarDesc := "foo.baz equals bar" | ||
|
||
var tests = []struct { | ||
description string | ||
body string | ||
condDesc string | ||
condConf *conditions.Config | ||
result bool | ||
}{ | ||
{ | ||
"positive match", | ||
"{\"foo\": {\"baz\": \"bar\"}}", | ||
fooBazEqualsBarDesc, | ||
fooBazEqualsBarConf, | ||
true, | ||
}, | ||
{ | ||
"Negative match", | ||
"{\"foo\": 123}", | ||
fooBazEqualsBarDesc, | ||
fooBazEqualsBarConf, | ||
false, | ||
}, | ||
{ | ||
"unparseable", | ||
`notjson`, | ||
fooBazEqualsBarDesc, | ||
fooBazEqualsBarConf, | ||
false, | ||
}, | ||
} | ||
|
||
for _, test := range tests { | ||
t.Run(test.description, func(t *testing.T) { | ||
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
fmt.Fprintln(w, test.body) | ||
})) | ||
defer ts.Close() | ||
|
||
res, err := http.Get(ts.URL) | ||
if err != nil { | ||
log.Fatal(err) | ||
} | ||
|
||
checker, err := checkJSON([]*jsonResponseCheck{{test.condDesc, test.condConf}}) | ||
require.NoError(t, err) | ||
checkRes := checker(res) | ||
|
||
if result := checkRes == nil; result != test.result { | ||
if test.result { | ||
t.Fatalf("Expected condition: '%s' to match body: %s. got: %s", test.condDesc, test.body, checkRes) | ||
} else { | ||
t.Fatalf("Did not expect condition: '%s' to match body: %s. got: %s", test.condDesc, test.body, checkRes) | ||
} | ||
} | ||
}) | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,6 +41,43 @@ def test_http(self, status_code): | |
raise SkipTest | ||
self.assert_fields_are_documented(output[0]) | ||
|
||
@parameterized.expand([ | ||
("up", '{"foo": {"baz": "bar"}}'), | ||
("down", '{"foo": "unexpected"}'), | ||
("down", 'notjson'), | ||
]) | ||
def test_http_json(self, expected_status, body): | ||
""" | ||
Test JSON response checks | ||
""" | ||
server = self.start_server(body, 200) | ||
try: | ||
self.render_config_template( | ||
monitors=[{ | ||
"type": "http", | ||
"urls": ["http://localhost:{}".format(server.server_port)], | ||
"check_response_json": [{ | ||
"description": "foo equals bar", | ||
"condition": { | ||
"equals": {"foo": {"baz": "bar"}} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does the YAML output look like here? I assume if you use Sorry to keep poking on this one. Agree it's a potential condition issue if it breaks but it shows up here very prominent. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did some investigation and determined that dotted paths and nested maps are equivalent here due to the use of I think we should move further discussion of this to a separate issue since this has more to do with the beats condition evaluation engine than this specific PR. |
||
} | ||
}] | ||
}] | ||
) | ||
|
||
try: | ||
proc = self.start_beat() | ||
self.wait_until(lambda: self.log_contains("heartbeat is running")) | ||
|
||
self.wait_until( | ||
lambda: self.output_has(lines=1)) | ||
finally: | ||
proc.check_kill_and_wait() | ||
|
||
self.assert_last_status(expected_status) | ||
finally: | ||
server.shutdown() | ||
|
||
@parameterized.expand([ | ||
(lambda server: "localhost:{}".format(server.server_port), "up"), | ||
# This IP is reserved in IPv4 | ||
|
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.
What is the purpose of the description? logging output?
As this is YAML I would more expect someone that wants to put details about it as a yaml comment.
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.
It's interpolated into the
error
field in the event when the check fails.This is important because it means the user can associate a human readable error with the downtime message when they get, say, an SMS.
So, instead of getting "http check for elastic.co/foo failed" they can get one that says
JSON body did not match condition '%s' for monitor. Received JSON %+v
, where%s
is the description.When using multiple conditions this is even more important since it may not be apparent what caused the failure.
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.
Got it. So the description is sent with every event or just errors (need to check the code).
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.
@ruflin no, it's only sent on error. There are multiple conditions checked, and the first one that fails is the only one that creates an error.
Now that I think about it, that's probably a mistake. We should run all the conditions and list all of the condition failures.
I'll improve the PR by making the error message include a list of error messages.
JSON body did not match 2 conditions: "version greater than 1", "name must be valid". Received JSON {...}