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

7403 - Allow to collect HTTP Headers thanks to http_response #7405

Merged
merged 1 commit into from
May 26, 2020
Merged

7403 - Allow to collect HTTP Headers thanks to http_response #7405

merged 1 commit into from
May 26, 2020

Conversation

essobedo
Copy link
Contributor

@essobedo essobedo commented Apr 24, 2020

fixes #7403

Required for all PRs:

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

@danielnelson
Copy link
Contributor

What do you think about making this look more similar to the new http_header_tags option we added to the http_listener_v2 plugin? We could name it response_header_tags, instead of fields it would produce a tag, and it would require the user to give a name for the tag.

@danielnelson danielnelson added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Apr 29, 2020
@essobedo
Copy link
Contributor Author

essobedo commented Apr 30, 2020

@danielnelson thx for the feedback, here is a new version based on what has been done in the http_listener_v2 plugin

@essobedo
Copy link
Contributor Author

essobedo commented May 6, 2020

@danielnelson could you please tell me if this new version of the PR is what you had in mind?

Copy link
Contributor

@danielnelson danielnelson left a comment

Choose a reason for hiding this comment

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

Looks good, just one minor change if you could

@@ -239,6 +245,14 @@ func (h *HTTPResponse) httpGather(u string) (map[string]interface{}, map[string]
resp, err := h.client.Do(request)
response_time := time.Since(start).Seconds()

// Add the response headers
for headerName, measurementName := range h.HTTPHeaderTags {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename measurementName -> tagKey or just tag. This will reduce confusion against the metric measurement/name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danielnelson done, please check again

@essobedo essobedo requested a review from danielnelson May 25, 2020 09:11
@danielnelson danielnelson added this to the 1.15.0 milestone May 26, 2020
@danielnelson danielnelson merged commit 7ef1d53 into influxdata:master May 26, 2020
@essobedo essobedo deleted the 7403/collect_http_headers branch May 27, 2020 07:17
@essobedo
Copy link
Contributor Author

@danielnelson thanks :-)

jaecktec pushed a commit to jaecktec/telegraf that referenced this pull request Jun 8, 2020
rhajek pushed a commit to bonitoo-io/telegraf that referenced this pull request Jul 13, 2020
idohalevi pushed a commit to idohalevi/telegraf that referenced this pull request Sep 29, 2020
arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow to collect HTTP Headers thanks to http_response
2 participants