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 generic http input plugin which supports setting an input data format #3546

Merged

Conversation

grange74
Copy link
Contributor

@grange74 grange74 commented Dec 6, 2017

Issue #813.

Required for all PRs:

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


## Mandatory data_format
## See available options at https://github.com/influxdata/telegraf/blob/master/docs/DATA_FORMATS_INPUT.md
data_format = "json"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually optional, if unset it will default to "influx". Just use the default snippet here:

  ## Data format to consume.
  ## Each data format has its own unique set of configuration options, read
  ## more about them here:
  ## https://github.com/influxdata/telegraf/blob/master/docs/DATA_FORMATS_INPUT.md
  # data_format = "influx"

var sampleConfig = `
## One or more URLs from which to read formatted metrics
urls = [
"http://localhost:2015/simple.json"
Copy link
Contributor

Choose a reason for hiding this comment

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

Because of my comment about data_format we should set this to something more format neutral such as http://localhost/metrics

}
h.client = &http.Client{
Transport: &http.Transport{
ResponseHeaderTimeout: h.Timeout.Duration,
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't set a response header timeout since we have the client timeout. I need to exercise these from the code so they stop getting copied around.


## http request & header timeout
## defaults to 5s if not set
timeout = "10s"
Copy link
Contributor

Choose a reason for hiding this comment

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

Update the comment based on removing header timeout (see below). Use the default value and comment it out:

# timeout = "5s"

@danielnelson
Copy link
Contributor

I may have mentioned that I'm hoping to deprecate the httpjson plugin. I looked through its feature set to see what we might still need. (None of these are required to have this merged)

The httpjson plugin has support for setting general headers, and also using the POST or GET method. I think these are important options we will need.

It also adds a tag server to every metric with the actual url. I think this is also important but it seems that you should be able to configure if you want this tag added, since it could conflict with data provided at the url. Perhaps we should have an option tag_url = false, and instead of calling this tag server we should call it url?

A float field is added to store the response_time. This one I don't think we should add, it doesn't fit the purpose of the plugin in my opinion. If there is strong demand for this then perhaps this could be a selfstat aka internal metric.

@danielnelson danielnelson added this to the 1.6.0 milestone Dec 6, 2017
@grange74
Copy link
Contributor Author

I have updated based on your 4 comments.
In the next few days, i will look to add support for setting generic headers, POST method and optional tagging with url.

@danielnelson danielnelson merged commit f82f03b into influxdata:master Feb 16, 2018
@danielnelson
Copy link
Contributor

I added a few things after merging, notably method support and I decided to change the url tagging to method to automatically tag with url if it is not set in the remote host. I decided to make this change because I felt like it was closer to the way the measurement name is set.

@grange74
Copy link
Contributor Author

@danielnelson Thanks for merging the PR and explaining your changes. The changes make perfect sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants