Add result_type field to net_response input plugin #2990

bobmshannon · 2017-07-06T01:55:34Z

This PR adds a newly proposed field called result_type (inspired by the http_response input plugin) which indicates whether a collection was either successful or failed. This should be useful for alerting purposes, and also will fix #2794. In an effort to be consistent it also removes the string_match boolean field and replaces it with a string_mismatch result type (this is a breaking change, not sure if we should keep this for backward compatibility or not).

Proposed result types:

result_type (string) # success, timeout, connection_failed, read_failed, string_mismatch

Test results

* Plugin: inputs.net_response, Collection 1
> net_response,server=influxdata.com,port=8080,protocol=tcp,host=localhost result_type="timeout" 1499310361000000000
* Plugin: inputs.net_response, Collection 1
> net_response,server=influxdata.com,port=443,protocol=tcp,host=localhost result_type="success",response_time=0.088703864 1499310361000000000
* Plugin: inputs.net_response, Collection 1
> net_response,protocol=tcp,host=localhost,server=this.domain.does.not.exist,port=443 result_type="connection_failed" 1499310361000000000
* Plugin: inputs.net_response, Collection 1
> net_response,protocol=udp,host=localhost,server=influxdata.com,port=8080 result_type="read_failed" 1499310362000000000
* Plugin: inputs.net_response, Collection 1
> net_response,port=31338,protocol=udp,host=localhost,server=localhost result_type="string_mismatch",string_found=false,response_time=0.00242682 1499310362000000000
* Plugin: inputs.net_response, Collection 1
> net_response,protocol=udp,host=localhost,server=localhost,port=31338 response_time=0.001128598,result_type="success",string_found=true 1499310362000000000
* Plugin: inputs.net_response, Collection 1
> net_response,server=this.domain.does.not.exist,port=443,protocol=udp,host=localhost result_type="connection_failed" 1499310362000000000

Required for all PRs:

Signed CLA.
Associated README.md updated.
Has appropriate unit tests.

Fix test Simplify states

danielnelson · 2017-07-06T18:21:06Z

Let's keep the old field around, but mark it deprecated in the documentation.

danielnelson · 2017-07-10T19:13:55Z

plugins/inputs/net_response/net_response.go

@@ -87,18 +92,21 @@ func (n *NetResponse) TcpGather() (map[string]interface{}, error) {
 		responseTime = time.Since(start).Seconds()
 		// Handle error
 		if err != nil {
-			fields["string_found"] = false
+			fields["result_type"] = "read_failed"


I think we should just report string_mismatch here.

danielnelson · 2017-07-10T19:14:26Z

plugins/inputs/net_response/net_response.go

 			} else {
-				fields["string_found"] = false
+				fields["result_type"] = "string_mismatch"
+				fields["string_found"] = false // WARNING: This field will be deprecated in a future release.


Don't add these comments, having it up in the README is enough.

danielnelson · 2017-07-10T19:15:16Z

plugins/inputs/net_response/README.md

@@ -59,7 +59,8 @@ It can also check response text.

 - net_response
    - response_time (float, seconds)
-    - string_found (bool) # Only if "expected: option is set
+    - result_type (string) # success, timeout, connection_failed, read_failed, string_mismatch
+    - [**DEPRECATED**] string_mismatch (boolean)


This name is wrong, should be string_found

Fix typo in README Remove deprecated comments Tweak Missed a few deprecated warnings

danielnelson · 2017-07-13T01:22:07Z

plugins/inputs/net_response/net_response.go

@@ -87,18 +92,21 @@ func (n *NetResponse) TcpGather() (map[string]interface{}, error) {
 		responseTime = time.Since(start).Seconds()
 		// Handle error
 		if err != nil {
-			fields["string_found"] = false


I think we still need this line so we don't change the existing item yet.

Oops -- yeah definitely. I'll take care of that.

danielnelson · 2017-07-13T01:28:16Z

plugins/inputs/net_response/net_response.go

+		} else {
+			fields["result_type"] = "connection_failed"
+		}
+		return fields, nil


Is it even possible to get in here with UDP?

I don't think so. The more generic "connection_failed" probably makes more sense here.

Or if the fact that UDP is connectionless is not consistent with such a name, perhaps add a new failure state. What do you think?

I think it would be a good starting point to just have connection_failed here, we can always add more specific states later if needed.

danielnelson · 2017-07-13T01:31:55Z

plugins/inputs/net_response/net_response.go

@@ -87,18 +92,21 @@ func (n *NetResponse) TcpGather() (map[string]interface{}, error) {
 		responseTime = time.Since(start).Seconds()
 		// Handle error
 		if err != nil {
-			fields["string_found"] = false
+			fields["result_type"] = "string_mismatch"


I know I told you to have this be string_mismatch, but I was wrong...it should be read_failed. Sorry.

No worries, will take care of it!

morfien101 · 2017-11-08T13:03:03Z

We are trying to use this result_type in grafana to create graphs that show trends. We would also like to alert if the connections fail.
For reference in the graphs it would be better if this was a tag so that we could hover over it to get information about the failure. result_type is a string so offers very little in terms of use when graphing data success and failures with a group of tests.

I am happy to be told that I am doing it wrong if there is another way to achieve what I want.

danielnelson · 2017-11-08T19:03:07Z

This is something we discussed when working on #3333, one issue with using a tag is that you must have at least one field.

morfien101 · 2017-11-08T19:17:20Z

@danielnelson In this case it could be quite easy.

As to maintain backwards compatibility you can leave the result_type as a field value but use exactly the same value for a tag. Or better use the value of the field and make a tag call something like "status".
The end result is that you would still always have a field available.

I can see the use of the field in a table, just not in a graph. However the exact same data in a tag would enable the creation of a graph.

danielnelson · 2017-11-08T23:54:25Z

I'm more inclined to also add this as a result_code integer field.

morfien101 · 2017-11-09T15:20:28Z

@danielnelson #3451 I have created a pull request that is green on tests that does what you suggested. Let me know if you are willing to consume it.

danielnelson · 2017-11-09T20:56:18Z

We can't move the field to a tag or change the type of the field since it will break compatibility, but if you want to add the field as an int under a different name result_code that's okay.

morfien101 · 2017-11-10T11:46:00Z

I have made a new PR with the changes suggested.
#3455

bobmshannon changed the title ~~Add result_type field to net_response input plugin~~ [proposal] Add result_type field to net_response input plugin Jul 6, 2017

bobmshannon force-pushed the enhancement/net_response_result_type_field branch from 4c289f6 to ad0470f Compare July 6, 2017 02:58

Add result_type field to net_response input plugin

a90f940

Fix test Simplify states

bobmshannon force-pushed the enhancement/net_response_result_type_field branch from ad0470f to a90f940 Compare July 6, 2017 03:05

Bobby Shannon added 2 commits July 5, 2017 23:12

Fix README title

59ab761

Fix lint

f3758b9

Bob Shannon added 4 commits July 7, 2017 19:44

Update example influx lines; re-add string_match as deprecated field

24d7891

string_match --> string_found

db45f70

Update test with deprecated field

ca03c6f

Fix lint

cff126b

bobmshannon changed the title ~~[proposal] Add result_type field to net_response input plugin~~ Add result_type field to net_response input plugin Jul 8, 2017

Update example output in README

f495f08

danielnelson suggested changes Jul 10, 2017

View reviewed changes

bobmshannon force-pushed the enhancement/net_response_result_type_field branch from 3f1b85c to 530ac6f Compare July 13, 2017 00:36

Remove read_failed state from TCP prober

925eeef

Fix typo in README Remove deprecated comments Tweak Missed a few deprecated warnings

bobmshannon force-pushed the enhancement/net_response_result_type_field branch from 530ac6f to 925eeef Compare July 13, 2017 00:38

danielnelson reviewed Jul 13, 2017

View reviewed changes

danielnelson added this to the 1.4.0 milestone Jul 13, 2017

Remove irrelevant UDP error check; re-add string_found field

9011ff6

danielnelson approved these changes Jul 14, 2017

View reviewed changes

danielnelson merged commit ef63908 into influxdata:master Jul 14, 2017

jeichorn pushed a commit to jeichorn/telegraf that referenced this pull request Jul 24, 2017

Add result_type field to net_response input plugin (influxdata#2990)

cac4e61

maxunt pushed a commit that referenced this pull request Jun 26, 2018

Add result_type field to net_response input plugin (#2990)

39a8f73

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add result_type field to net_response input plugin #2990

Add result_type field to net_response input plugin #2990

bobmshannon commented Jul 6, 2017 •

edited

Loading

danielnelson commented Jul 6, 2017

danielnelson Jul 10, 2017

danielnelson Jul 10, 2017

danielnelson Jul 10, 2017

danielnelson Jul 13, 2017

bobmshannon Jul 13, 2017

danielnelson Jul 13, 2017

bobmshannon Jul 13, 2017

bobmshannon Jul 13, 2017

danielnelson Jul 13, 2017

danielnelson Jul 13, 2017

bobmshannon Jul 13, 2017

morfien101 commented Nov 8, 2017

danielnelson commented Nov 8, 2017 •

edited

Loading

morfien101 commented Nov 8, 2017

danielnelson commented Nov 8, 2017

morfien101 commented Nov 9, 2017 •

edited

Loading

danielnelson commented Nov 9, 2017

morfien101 commented Nov 10, 2017

Add result_type field to net_response input plugin #2990

Add result_type field to net_response input plugin #2990

Conversation

bobmshannon commented Jul 6, 2017 • edited Loading

Test results

Required for all PRs:

danielnelson commented Jul 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morfien101 commented Nov 8, 2017

danielnelson commented Nov 8, 2017 • edited Loading

morfien101 commented Nov 8, 2017

danielnelson commented Nov 8, 2017

morfien101 commented Nov 9, 2017 • edited Loading

danielnelson commented Nov 9, 2017

morfien101 commented Nov 10, 2017

bobmshannon commented Jul 6, 2017 •

edited

Loading

danielnelson commented Nov 8, 2017 •

edited

Loading

morfien101 commented Nov 9, 2017 •

edited

Loading