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

Add a generic HTTPPost node #1331

Merged
merged 2 commits into from
Apr 25, 2017
Merged

Add a generic HTTPPost node #1331

merged 2 commits into from
Apr 25, 2017

Conversation

sputnik13
Copy link
Contributor

@sputnik13 sputnik13 commented Apr 19, 2017

An HTTPPost node to mimic the behavior of HTTPOut, with the difference
that data is POST'd to an HTTP endpoint rather than made available at a
kapacitor endpoint.

Closes #1330

Tested to ensure data is POST'd properly to a logstash instance, from which
metric data was pushed through our metric pipeline to a time series database.

Integration tests for stream and batch modes added and verified.

Required for all non-trivial PRs
  • Rebased/mergable
  • Tests pass
  • CHANGELOG.md updated
  • Sign CLA (if not already signed)
Required only if applicable

You can erase any checkboxes below this note if they are not applicable to your Pull Request.

@nathanielc
Copy link
Contributor

nathanielc commented Apr 19, 2017

@sputnik13 On first read through this look great! 👍

Questions:

  • Do you need support for authentication? Meaning using basic auth, to authenticate against the target.
  • Do you need the ability to add headers to the POST request?

I ask because both of these have been requested and are being added in #1254, for the alert post handler.

@desa Seems like we can use your work in #1254 to add the same features to this new httpPost node. Just change alertpost service to httppost service and use it for both. Thoughts?

@desa
Copy link
Contributor

desa commented Apr 20, 2017

@nathanielc yeah I think that makes sense. Thoughts on the best way to do that? Should we get this merged and then I'll rebase and make the changes?

@nathanielc
Copy link
Contributor

@desa Yeah, I think it makes sense to merge this one first. Thanks

// .every(5s)
// |top('value', 10)
// //Post the top 10 results over the last 10s updated every 5s.
// |httpPost('http://example.com/api/top10')
Copy link
Contributor

Choose a reason for hiding this comment

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

@desa Does this TICKscript usage work well for how your post service is going to work? Is there a change that would make is easier more consistent with your changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

having an optional property method called endpoint would be useful, but I can add that once this is merged.

Copy link
Contributor

@nathanielc nathanielc left a comment

Choose a reason for hiding this comment

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

@sputnik13 Thanks for the excellent PR! I am ready to merge this once @desa also approves that it will integrate well with his work.

@sputnik13
Copy link
Contributor Author

@nathanielc sounds good, except during further internal testing we saw kapacitor hard crash when the POST endpoint wasn't available... not sure that's the best behavior, so I need to look in to this a bit more

An HTTPPost node to mimic the behavior of HTTPOut, with the difference
that data is POST'd to an HTTP endpoint rather than made available at a
kapacitor endpoint.

Closes influxdata#1330
@sputnik13
Copy link
Contributor Author

sputnik13 commented Apr 20, 2017

@nathanielc it seems like closing resp.Body when http.Post fails causes a hard crash (doh)... fixed

when http.Post fails, resp.Body.Close seems to cause a hard crash on the
entire process
@desa
Copy link
Contributor

desa commented Apr 25, 2017

@sputnik13 is this PR ready to go? If so, could you squash and rebase onto master?

@desa desa merged commit 5bace5c into influxdata:master Apr 25, 2017
@sputnik13
Copy link
Contributor Author

@desa I'm sorry, I was busy with work, hadn't had a chance to check back, thanks for merging it

@desa
Copy link
Contributor

desa commented Apr 26, 2017

@sputnik13 no worries 👍 . Thanks for the PR.

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.

3 participants