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

Implementing graphite protocol metricbeat module #4734

Merged
merged 2 commits into from
Jul 27, 2017
Merged

Implementing graphite protocol metricbeat module #4734

merged 2 commits into from
Jul 27, 2017

Conversation

vjsamuel
Copy link
Contributor

No description provided.

@elasticmachine
Copy link
Collaborator

Can one of the admins verify this patch?

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically on build-eu-00. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@tsg tsg added Metricbeat Metricbeat review labels Jul 24, 2017
exekias
exekias reviewed Jul 24, 2017
View reviewed changes
Copy link
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @vjsamuel! I just did a first pass, the module is looking good, some tests would help :)

metricbeat/docs/modules/http.asciidoc Outdated
@@ -42,5 +42,9 @@ The following metricsets are available:

* <<metricbeat-metricset-http-json,json>>

* <<metricbeat-metricset-http-server,server>>
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 http server metricset is spurious previous to a rename to graphite? also present here: https://github.com/elastic/beats/pull/4734/files#diff-8303434bf263f758175d97b66f919690R48

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was a wrong checkin from another metricset :) will follow in a subsequent PR. i have removed it from this one.

metricbeat/helper/server/tcp/tcp.go Outdated
conn.Close()
}
}()
buffer := make([]byte, g.receiveBufferSize)
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be optimized out of the loop, what do you think=

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

metricbeat/helper/server/udp/udp.go Outdated
func (g *UdpServer) WatchMetrics() {

for {
buffer := make([]byte, g.receiveBufferSize)
Copy link
Contributor

Choose a reason for hiding this comment

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

same thing for this buffer

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

metricbeat/module/graphite/server/data.go Outdated
}

func NewMetricProcessor(templates []templateConfig, defaultTemplate templateConfig) metricProcessor {

Copy link
Contributor

Choose a reason for hiding this comment

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

minor thing, but recently we have been trying to avoid new lines at the start and end of functions: #4670

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

@vjsamuel vjsamuel force-pushed the graphite_protocol branch 8 times, most recently from c3a5056 to 06c17cf Compare July 25, 2017 02:59
@tsg tsg mentioned this pull request Jul 25, 2017
Closed
@exekias
Copy link
Contributor

exekias commented Jul 25, 2017

Check is failing because of goimports format, you can do make fmt to fix it

exekias
exekias reviewed Jul 25, 2017
View reviewed changes
metricbeat/module/graphite/_meta/config.yml
@@ -0,0 +1,4 @@
- module: graphite
metricsets: ["server"]
enabled: true
Copy link
Contributor

@exekias exekias Jul 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

What do you think about adding some (commented) template examples here?

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

@vjsamuel vjsamuel force-pushed the graphite_protocol branch 3 times, most recently from b65ae24 to 63711bf Compare July 25, 2017 16:59
@exekias
Copy link
Contributor

exekias commented Jul 25, 2017

jenkins test it please

@vjsamuel vjsamuel force-pushed the graphite_protocol branch 3 times, most recently from b4f4080 to e5e4423 Compare July 26, 2017 04:42
@exekias
Copy link
Contributor

exekias commented Jul 26, 2017

jenkins test it please

@vjsamuel vjsamuel force-pushed the graphite_protocol branch from e5e4423 to b28ceb6 Compare July 26, 2017 14:56
@exekias
Copy link
Contributor

exekias commented Jul 26, 2017

jenkins test it please

1 similar comment
@exekias
Copy link
Contributor

exekias commented Jul 26, 2017

jenkins test it please

@exekias exekias merged commit 38eb3eb into elastic:master Jul 27, 2017
@vjsamuel vjsamuel deleted the graphite_protocol branch July 27, 2017 04:58
vjsamuel added a commit to vjsamuel/beats that referenced this pull request Jul 31, 2017
@vjsamuel
Implementing graphite protocol metricbeat module (elastic#4734)
f821e13 
* Implementing graphite protocol metricbeat module
exekias pushed a commit that referenced this pull request Aug 1, 2017
@vjsamuel @exekias
Implementing graphite protocol metricbeat module (#4734) (#4770)
eb92974 
* Implementing graphite protocol metricbeat module
@exekias exekias added the needs_backport PR is waiting to be backported to other branches. label Aug 1, 2017
@tsg tsg removed the needs_backport PR is waiting to be backported to other branches. label Aug 24, 2017
exekias pushed a commit to exekias/beats that referenced this pull request Aug 24, 2017
@vjsamuel @exekias
Implementing graphite protocol metricbeat module (elastic#4734) (elas…
dc2ddd0 
…tic#4770)

* Implementing graphite protocol metricbeat module
(cherry picked from commit eb92974)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@exekias exekias exekias left review comments

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

Successfully merging this pull request may close these issues.
4 participants
@vjsamuel @elasticmachine @exekias @tsg