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.

Sign up for GitHub

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

Jump to bottom

Http module improvements #4170

Merged
merged 1 commit into from
May 4, 2017
Merged

Http module improvements #4170

merged 1 commit into from
May 4, 2017

Conversation

ruflin
Copy link
Member

@ruflin ruflin commented May 2, 2017

This is a follow up PR for #4156

  • Add environment with small golang web service
  • Remove not needed body fields
  • Add defaults to configs
  • Add system tests
  • Update config with all config options

@ruflin ruflin mentioned this pull request May 2, 2017
Merged
@ruflin
Copy link
Member Author

ruflin commented May 2, 2017

jenkins, retest it

tsg
tsg reviewed May 2, 2017
View reviewed changes
metricbeat/docs/modules/http.asciidoc Outdated
Http module is a [Metricbeat](https://www.elastic.co/products/beats/metricbeat) used to call arbitrary HTTP endpoints for which not a dedicated metricbeat is available.
Multiple endpoints can be configured which are polled in a regular interval and the result is shipped to the configured output channel.
Http module is a [Metricbeat](https://www.elastic.co/products/beats/metricbeat) used to call arbitrary HTTP endpoints for which not a dedicated metricbeat module is available.
Multiple endpoints can be configured which are polled in a regular interval and the result is shipped to the configured output channel. It is recommend to fetch install a metricbeat instance on each host from which data should be fetched.
Copy link
Contributor

Choose a reason for hiding this comment

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

Some typos in this phrase: "It is recommended to (fetch) install..."

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

@ruflin It still displays the old format "It is recommend to fetch install".

tsg
tsg reviewed May 2, 2017
View reviewed changes
metricbeat/docs/modules/http.asciidoc Outdated
@@ -5,8 +5,8 @@ This file is generated! See scripts/docs_collector.py
[[metricbeat-module-http]]
== http Module

Http module is a [Metricbeat](https://www.elastic.co/products/beats/metricbeat) used to call arbitrary HTTP endpoints for which not a dedicated metricbeat is available.
Multiple endpoints can be configured which are polled in a regular interval and the result is shipped to the configured output channel.
Http module is a [Metricbeat](https://www.elastic.co/products/beats/metricbeat) used to call arbitrary HTTP endpoints for which not a dedicated metricbeat module is available.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's a missing "module" after Metricbeat, and an extra "not".

Copy link
Member Author

Choose a reason for hiding this comment

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

changed to module used to call arbitrary HTTP endpoints for which a dedicated metricbeat module is not available.

tsg
tsg reviewed May 2, 2017
View reviewed changes
metricbeat/docs/modules/http.asciidoc
@@ -5,8 +5,8 @@ This file is generated! See scripts/docs_collector.py
[[metricbeat-module-http]]
== http Module
Copy link
Contributor

Choose a reason for hiding this comment

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

"http Module" looks weird in the docs due to capitalization. I think there was a way to define the title in fields.yml?

Copy link
Member Author

Choose a reason for hiding this comment

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

changed the title in fields.yml to HTTP.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would use HTTP for the protocol, and http for the module name. What about http module?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, sorry. I just noticed that you changed already to Http module. That's perfect.

@ruflin
Copy link
Member Author

ruflin commented May 2, 2017

Review comments applied and pushed.

@ruflin
Copy link
Member Author

ruflin commented May 2, 2017

jenkins, retest it

monicasarbu
monicasarbu reviewed May 2, 2017
View reviewed changes
metricbeat/module/http/_meta/docs.asciidoc Outdated
Http module is a [Metricbeat](https://www.elastic.co/products/beats/metricbeat) used to call arbitrary HTTP endpoints for which not a dedicated metricbeat is available.
Multiple endpoints can be configured which are polled in a regular interval and the result is shipped to the configured output channel.
Http module is a [Metricbeat](https://www.elastic.co/products/beats/metricbeat) used to call arbitrary HTTP endpoints for which not a dedicated metricbeat module is available.
Multiple endpoints can be configured which are polled in a regular interval and the result is shipped to the configured output channel. It is recommend to fetch install a metricbeat instance on each host from which data should be fetched.
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't sound good to me "to fetch install", maybe just "to install".

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that Tudor left the same comment above.

Copy link
Member Author

Choose a reason for hiding this comment

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

Argh, I didn't push the change.

@ruflin
Http module improvements
81f071d 
This is a follow up PR for elastic#4156

* Add environment with small golang web service
* Remove not needed body fields
* Add defaults to configs
* Add system tests
* Update config with all config options
@monicasarbu monicasarbu merged commit c1132fa into elastic:master May 4, 2017
@monicasarbu monicasarbu deleted the http-module-cleanup branch May 4, 2017 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@monicasarbu monicasarbu monicasarbu left review comments

@tsg tsg tsg left review comments

@exekias exekias exekias approved these changes

Assignees
No one assigned
Labels
Metricbeat Metricbeat refactoring review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ruflin @tsg @exekias @monicasarbu