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

Http check additions #375

Merged
merged 5 commits into from
Feb 22, 2013
Merged

Http check additions #375

merged 5 commits into from
Feb 22, 2013

Conversation

dcrosta
Copy link
Contributor

@dcrosta dcrosta commented Feb 14, 2013

Add (a truncated copy of) the HTTP body on failures to the alert messages generated by http_check.

This probably only works helpfully for "health check" endpoints which return a brief message about why they failed; human-friendly HTML pages (like 404s) probably won't be that usable in 200 characters or less. It would be fairly simple to add a configuration option for whether to include the body or not.

@ghost ghost assigned remh Feb 15, 2013
@alq666
Copy link
Member

alq666 commented Feb 15, 2013

@remh what do you think?

@@ -84,6 +84,16 @@ def _create_status_event(self, status, msg, instance):
notify_message = " ".join(notify_list) + " \n"

if status == Status.DOWN:
# format the HTTP response body into the event
code, reason, content = msg
Copy link
Contributor

Choose a reason for hiding this comment

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

This code will fail with a ValueError in case we got a Status.DOWN which was not raised by a response.code > 400 (e.g. a socket timeout: https://github.com/dcrosta/dd-agent/blob/c111602b02ceae75f1447dc2986489e07e58818d/checks.d/http_check.py#L32) as it won't return a triplet

Copy link
Contributor

Choose a reason for hiding this comment

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

(See the travis build: https://travis-ci.org/DataDog/dd-agent/jobs/4804845 for an example of failure)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yikes, good point. Thanks for catching this.

@remh
Copy link
Contributor

remh commented Feb 15, 2013

It's a good idea, it needs a few fixes though but it's definitely a good thing to have.

@remh
Copy link
Contributor

remh commented Feb 15, 2013

Thanks a lot @dcrosta by the way ! :)

@dcrosta
Copy link
Contributor Author

dcrosta commented Feb 15, 2013

@remh @alq666 how do you feel about adding a flag to enable this behavior?

@dcrosta
Copy link
Contributor Author

dcrosta commented Feb 15, 2013

That is, a flag in the conf.d/http_check.yaml file -- something like include_response: true

@remh
Copy link
Contributor

remh commented Feb 20, 2013

@dcrosta : I agree, the response should only be included if the flag is on as 200 characters of plain html code will be hard to read.
Can you add it (yes in the yaml file) and set its default value to False ?
Thanks a lot it looks good otherwise! 👍

@dcrosta
Copy link
Contributor Author

dcrosta commented Feb 20, 2013

Done :)

content = content[:197] + '...'
content = content.replace('<', '&lt;').replace('>', '&gt;')

msg = "%d %s\n\n%s" % (code, reason, content)
Copy link
Contributor

Choose a reason for hiding this comment

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

last thing before we merge.
Why do you need this line ?
With this line the created event on datadog is full of < instead of proper < >.
It will be good to merge once it's removed! Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which line?

If you mean L98: I wasn't sure if datadog will escape the HTML on display or not, and wanted to be safe. If it does do this, then, yes, we should remove it.

If you mean L100 -- then I'm not sure I understand the question.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes L98. Sorry I put the comment on the wrong line :$

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. And DataDog's web UI escapes the HTML (including possibly malformed HTML due to truncation) properly?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@remh
Copy link
Contributor

remh commented Feb 22, 2013

Thanks a lot @dcrosta 👍

remh added a commit that referenced this pull request Feb 22, 2013
@remh remh merged commit 2e719ef into DataDog:master Feb 22, 2013
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.

None yet

3 participants