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

Adding dropwizard module #4022

Merged
merged 3 commits into from Apr 20, 2017

Conversation

Projects
None yet
5 participants
@vjsamuel
Contributor

vjsamuel commented Apr 16, 2017

Adding metricbeat module that can query HTTP servlets that expose dropwizard metrics.

cc: @Randgalt

@elasticmachine

This comment has been minimized.

elasticmachine commented Apr 16, 2017

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

1 similar comment
@elasticmachine

This comment has been minimized.

elasticmachine commented Apr 16, 2017

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

// Part of new is also setting up the configuration by processing additional
// configuration entries if needed.
func New(base mb.BaseMetricSet) (mb.MetricSet, error) {
logp.Warn("BETA: The dropwizard collector metricset is beta")

This comment has been minimized.

@exekias

exekias Apr 17, 2017

Member

We have logp.Beta for this

This comment has been minimized.

@vjsamuel

vjsamuel Apr 18, 2017

Contributor

done

@exekias

This comment has been minimized.

Member

exekias commented Apr 17, 2017

LGTM apart from a minor comment, please also add a changelog

@vjsamuel

This comment has been minimized.

Contributor

vjsamuel commented Apr 18, 2017

@ruflin @exekias done with review comments and added integration test.

@ruflin

In general LGTM. Left a few minor comments. For the dropwizard integration tests docker image: For me this is not only for automated testing but also having an easy way in case things don't work, for us to manually test with an actual dropwizard app to reproduce potential problems.

[[metricbeat-module-dropwizard]]
== dropwizard Module
This is the dropwizard Module.

This comment has been minimized.

@ruflin

ruflin Apr 18, 2017

Collaborator

Could you add a link to dropwizard here?

This comment has been minimized.

@vjsamuel

vjsamuel Apr 18, 2017

Contributor

done.

"net/http"
)
var response = `{"version":"4.0.0","gauges":{"my_gauge{this=that}":{"value":565}},"counters":{"my_counter":{"count":565},"my_counter2{this=that}":{"count":565}},"histograms":{"my_hist":{"count":565,"max":564,"mean":563.3148706761577,"min":0,"p50":564.0,"p75":564.0,"p95":564.0,"p98":564.0,"p99":564.0,"p999":564.0,"stddev":1.0747916190718627}},"meters":{"my_meter":{"count":0,"m1_rate":0.0,"m5_rate":0.0,"m15_rate":0.0,"mean_rate":0.0,"units":"events/second"}},"timers":{"my_timer{this=that}":{"count":0,"max":0.0,"mean":0.0,"min":0.0,"p50":0.0,"p75":0.0,"p95":0.0,"p98":0.0,"p99":0.0,"p999":0.0,"stddev":0.0,"m1_rate":0.0,"m5_rate":0.0,"m15_rate":0.0,"mean_rate":0.0,"duration_units":"seconds","rate_units":"calls/second"}}}`

This comment has been minimized.

@ruflin

ruflin Apr 18, 2017

Collaborator

For testing the JSON output we don't need the integration tests. We could also do this with eventMapping and just load the content.

I would like to have a docker environment with a real dropwizard app inside (if possible). So in case things break, we have an instance around to test and don't have to build one ourself.

This comment has been minimized.

@vjsamuel

vjsamuel Apr 18, 2017

Contributor

added a proper dropwizard http endpoint as part of the integration run.

"version": "6.0.0-alpha1"
},
"dropwizard": {
"test": {

This comment has been minimized.

@ruflin

ruflin Apr 18, 2017

Collaborator

As it is a "dynamic" metricset I'm a bit undecided what we should do here. In other dynamic metricsets we leave it empty here (https://github.com/elastic/beats/blob/master/metricbeat/module/windows/perfmon/_meta/data.json) but provide an example output in the docs.

This comment has been minimized.

@vjsamuel

vjsamuel Apr 18, 2017

Contributor

removed the custom metrics

dw := map[string]interface{}{}
d := json.NewDecoder(strings.NewReader(string(body)))
d.UseNumber()

This comment has been minimized.

@ruflin

ruflin Apr 18, 2017

Collaborator

Interesting. Is there something special about the floats returned so we need this?

This comment has been minimized.

@vjsamuel
@@ -7,3 +7,4 @@ _meta/beat.full.yml
/metricbeat.test
/docs/html_docs
target/

This comment has been minimized.

@ruflin

ruflin Apr 19, 2017

Collaborator

I assume that is coming from java?

This comment has been minimized.

@vjsamuel

vjsamuel Apr 19, 2017

Contributor

Yes. Just as a precaution in case someone tries to build the src locally.

This comment has been minimized.

@vjsamuel

vjsamuel Apr 19, 2017

Contributor

Yes. Just as a precaution in case someone tries to build the src locally.

This comment has been minimized.

@ruflin

ruflin Apr 19, 2017

Collaborator

Wouldn't that directory be inside the test folder?

This comment has been minimized.

@vjsamuel

vjsamuel Apr 19, 2017

Contributor

Yes. As long as there is no other directory named target we shouldn't have any issues.

This comment has been minimized.

@ruflin

ruflin Apr 19, 2017

Collaborator

In this case I would suggest to create a .gitignore file inside the dropwizard module.

This comment has been minimized.

@vjsamuel

vjsamuel Apr 19, 2017

Contributor

done.

@andrewkroh

This comment has been minimized.

Member

andrewkroh commented Apr 19, 2017

jenkins, test it

@ruflin ruflin merged commit f3f189c into elastic:master Apr 20, 2017

5 checks passed

codecov/patch 66.08% of diff hit (target 63.93%)
Details
codecov/project 63.95% (+0.01%) compared to 08ecc47
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Build finished.
Details
@ruflin

This comment has been minimized.

Collaborator

ruflin commented Apr 20, 2017

@vjsamuel Thanks a lot for the contribution.

@vjsamuel vjsamuel deleted the vjsamuel:dropwizard_module branch May 2, 2017

athom added a commit to athom/beats that referenced this pull request Jan 25, 2018

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