Add result_type field for http_response input #2814

froth · 2017-05-16T11:19:38Z

Does return measurement data even if no http connection can be established.

This is based on pull request #2813, as it depends on the fixes.

Required for all PRs:

CHANGELOG.md updated (we recommend not updating this until the PR has been approved by a maintainer)
Sign CLA (if not already signed)

phemmer · 2017-05-16T12:34:05Z

plugins/inputs/http_response/http_response.go

 		if h.FollowRedirects {
-			return nil, err
+			log.Printf("E! Failed to open HTTP connection: %s", err)
+			return fields, nil


Why are we directly logging the error instead of returning it?

The other option would be to give the the fields and the errors one level up and logging the errors there. I thought about that and my reasoning to do it this way is the following:
Currently we either return the fields or the error and this would break this pattern. In my opinion the way I chose is a little easier to understand.

However I don't have a strong opinion on it. I can change it, if it's important for you.

The reason I bring it up is 2 things. Doing it this way results in the log messages not having context. Meaning that if you have multiple inputs defined, you don't know which one is generating the error. The other problem is that I think we have an open issue somewhere to track statistics on per-input error counts, which requires errors going through Accumulator.AddError().

danielnelson · 2017-05-16T23:05:28Z

Check out my comment on #2784.

froth · 2017-05-17T07:26:17Z

@phemmer I amended my commit to reflect your comments. Can you have a look?

froth · 2017-05-22T12:45:51Z

I updated the pull request to reflect the discussion in #2784

danielnelson · 2017-05-23T21:12:05Z

plugins/inputs/http_response/http_response.go

 		if h.FollowRedirects {
-			return nil, err
+			return fields, err


In general, returning an error indicates that the value is not valid, if we are returning fields then we shouldn't have an error. Later on, we will need to make sure we don't call AddFields if it is nil.

danielnelson · 2017-05-23T21:15:12Z

plugins/inputs/http_response/http_response.go

 	if err != nil {
+		fields["http_response_code"] = -1
+		fields["response_string_match"] = 0


There was no response so these fields should not be set.

danielnelson · 2017-05-23T21:19:01Z

plugins/inputs/http_response/http_response.go

+		if netErr, ok := err.(net.Error); ok && netErr.Timeout() {
+			fields["response_timeout"] = time.Since(start).Seconds()
+		} else {
+			fields["response_time"] = time.Since(start).Seconds()


I think this goes away too, there was no response.

froth · 2017-05-24T07:21:40Z

Thanks for your comments. I updated the pull request to reflect them :)

danielnelson · 2017-05-24T17:57:37Z

plugins/inputs/http_response/http_response.go

@@ -148,6 +154,7 @@ func (h *HTTPResponse) httpGather() (map[string]interface{}, error) {

 	fields["response_time"] = time.Since(start).Seconds()
 	fields["http_response_code"] = resp.StatusCode
+	fields["connection_failed"] = 0


@phemmer What do you think, should we return this field if everything went okay?

My thoughts:
It looks like we have 3 different fields that behave similarly, connection_failed, response_timeout, and response_string_match. This feels wrong to me. All three fields are exclusive of each other. You'll never have more than one of them true. So it seems like we should instead have one field which indicates the result. Something like result_type with the values connection failed/timeout/response mismatch/response match.

If we do keep them separate fields, I would at least argue that they should be booleans, not ints.
And that I think the field should be emitted, just set to false.

One consequence of combining them is that we would not be able to store how long the timeout took, but maybe we don't care about this since it is basically set by hand.

Interesting idea to work match/mismatch in, do you think we would leave the field off if no match string is provided? Perhaps we have a success result_type?

I was letting the int slide here because this plugin already has the response match field with one, but yeah in general we should use booleans.

I think it is a good idea to always return a status whether the connection failed or not. If someone (i.e. me) wants to gather the latest value of the status and it is only returned when an error occured the value is always "error".

Do we have a plan if we want to combine the fields and if so: how?

I can change the int fields to boolean if you want, however that would mean a breaking change for the response_match field.

Sorry for the late reply, we had public holidays in Germany and I had to go rock climbing ;)

danielnelson · 2017-05-24T17:58:29Z

plugins/inputs/http_response/http_response.go

 	}
-	// Add metrics
-	acc.AddFields("http_response", fields, tags)


Should be able to remove both of these hunks, returning an error works the same as AddError with return nil.

froth · 2017-05-31T10:19:29Z

@danielnelson, @phemmer
I fixed the return error part.

Do we have a plan for the "connection_failed" field?
The way I have implemented it would fix the concrete problem I have. If you have input what can be done better I am willing to improve it further.

Currently I have the feeling that we are kind of stuck in the discussion of how to deal with the flag-fields. I think it is an important discussion and I appreciate that you take it seriously. But to be honest my current main concern is "getting the bug fixed soon, so that my alerting system is reliable".

danielnelson · 2017-06-01T00:05:19Z

As usual, @phemmer's solution is quite a bit better than the one i suggested earlier, so how about:

Forget about the response_timeout field for now, it's not clear if we need it and we can always add it later.
Add result_type field with possible values connection_failed, timeout, response_string_mismatch, success. This will replace the connection_failed field we had earlier and should be more extensible.

Even with this change, it would probably be a good idea to make sure you are alerting if the measurement you are looking for stops being reported.

froth · 2017-06-01T11:54:45Z

Done :)

Please have a look

danielnelson · 2017-06-01T21:59:25Z

Leave the old response_string_mismatch field around so we don't break anything.

froth · 2017-06-06T06:28:56Z

@danielnelson done :)

danielnelson · 2017-06-07T00:08:47Z

@froth It's in! Thanks and sorry about all the backtracking while we figured out what to do.

froth force-pushed the bugfix_http_timeout branch from 82fc8c8 to 9d523b6 Compare May 16, 2017 11:42

phemmer reviewed May 16, 2017

View reviewed changes

froth force-pushed the bugfix_http_timeout branch from 9d523b6 to 5e37c54 Compare May 17, 2017 07:24

froth force-pushed the bugfix_http_timeout branch from 5e37c54 to 3847faf Compare May 22, 2017 12:42

froth mentioned this pull request May 22, 2017

http_response does not measure anything on dns-error #2784

Closed

froth force-pushed the bugfix_http_timeout branch from 3847faf to 585326b Compare May 23, 2017 07:33

danielnelson reviewed May 23, 2017

View reviewed changes

froth force-pushed the bugfix_http_timeout branch from 585326b to f5cfa58 Compare May 24, 2017 07:21

froth force-pushed the bugfix_http_timeout branch from f5cfa58 to a43a168 Compare May 24, 2017 07:58

danielnelson reviewed May 24, 2017

View reviewed changes

froth force-pushed the bugfix_http_timeout branch from a43a168 to 727a7b9 Compare May 31, 2017 08:50

froth force-pushed the bugfix_http_timeout branch from 727a7b9 to de3b6c4 Compare June 1, 2017 11:44

froth force-pushed the bugfix_http_timeout branch from de3b6c4 to 683725a Compare June 1, 2017 11:57

danielnelson added this to the 1.4.0 milestone Jun 1, 2017

http_response: Measure data in case of timeout or dns error

c4fa8f4

froth force-pushed the bugfix_http_timeout branch from 683725a to c4fa8f4 Compare June 6, 2017 06:18

danielnelson changed the title ~~input/http_response: Bugfix for http timeout~~ Add result_type field for http_response input Jun 6, 2017

danielnelson merged commit 91f2764 into influxdata:master Jun 6, 2017

froth deleted the bugfix_http_timeout branch June 8, 2017 07:37

bobmshannon mentioned this pull request Jul 6, 2017

Add result_type field to net_response input plugin #2990

Merged

3 tasks

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

Add result_type field for http_response input (influxdata#2814)

dc439e2

danielnelson mentioned this pull request Aug 4, 2017

[inputs.http_response] Error in plugin if connection refused #3095

Closed

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

Add result_type field for http_response input (#2814)

38b7609

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add result_type field for http_response input #2814

Add result_type field for http_response input #2814

froth commented May 16, 2017

phemmer May 16, 2017

froth May 16, 2017 •

edited

Loading

phemmer May 16, 2017

danielnelson commented May 16, 2017 •

edited

Loading

froth commented May 17, 2017

froth commented May 22, 2017

danielnelson May 23, 2017

danielnelson May 23, 2017

danielnelson May 23, 2017

froth commented May 24, 2017

danielnelson May 24, 2017

phemmer May 24, 2017 •

edited

Loading

danielnelson May 24, 2017

froth May 29, 2017 •

edited

Loading

danielnelson May 24, 2017

froth commented May 31, 2017

danielnelson commented Jun 1, 2017

froth commented Jun 1, 2017

danielnelson commented Jun 1, 2017

froth commented Jun 6, 2017

danielnelson commented Jun 7, 2017

Add result_type field for http_response input #2814

Add result_type field for http_response input #2814

Conversation

froth commented May 16, 2017

Required for all PRs:

Choose a reason for hiding this comment

froth May 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielnelson commented May 16, 2017 • edited Loading

froth commented May 17, 2017

froth commented May 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

froth commented May 24, 2017

Choose a reason for hiding this comment

phemmer May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

froth May 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

froth commented May 31, 2017

danielnelson commented Jun 1, 2017

froth commented Jun 1, 2017

danielnelson commented Jun 1, 2017

froth commented Jun 6, 2017

danielnelson commented Jun 7, 2017

froth May 16, 2017 •

edited

Loading

danielnelson commented May 16, 2017 •

edited

Loading

phemmer May 24, 2017 •

edited

Loading

froth May 29, 2017 •

edited

Loading