Generic jmx importing #401

Merged
merged 6 commits into from Nov 8, 2016

Projects

None yet

2 participants

@hunterpayne-ck
Contributor

This is new functionality for Kamon. It allows importing of custom JMX metrics. The JMX metrics to import are specified in a config file and pumped into kamon just like any other metric. Then those metrics can be sent to any Kamon backend. Great for places that use Kamon and also older systems (like Kafka) that export their metrics via JMX. Also, many other parts of Kamon could be refactored to use this module instead of using custom code to access specific JMX metrics.

@dpsoft

@hunterpayne-ck I really like the idea!!! I left some comments and I think that we need some tests and an example of configuration.. and of course doc in our site WDYT?

@@ -0,0 +1,362 @@
+
@dpsoft
dpsoft Oct 13, 2016 Contributor

@hunterpayne-ck please add the kamon copyright in each file

+ val metricType: MetricTypeEnum.MetricType, val name: String,
+ unitOfMeasure: UnitOfMeasurement, // = UnitOfMeasurement.Unknown,
+ range: DynamicRange = null,
+ refreshInterval: FiniteDuration = null,
@dpsoft
dpsoft Oct 13, 2016 Contributor

why you use null values instead of Options values?

@hunterpayne-ck
Contributor

Great, thank you.

Use of null is probably an oversight. I often switch between Java and
Scala so sometimes I forget that one.
I will add the copywrite notices on my next commit. Probably on Monday.

Also, the Kamon unit tests seem to fail intermittently on tests in modules
I haven't touched. Is this normal? If not, what likely am I do wrong?

Hunter

On Thu, Oct 13, 2016 at 12:46 PM, Diego Parra notifications@github.com
wrote:

@dpsoft commented on this pull request.

@hunterpayne-ck https://github.com/hunterpayne-ck I really like the
idea!!! I left some comments and I think that we need some tests and an

example of configuration.. and of course doc in our site WDYT?

In kamon-jmx/src/main/scala/kamon/jmx/extension/ExportedMBeanQuery.scala
#401 (review):

@@ -0,0 +1,362 @@
+

@hunterpayne-ck https://github.com/hunterpayne-ck please add the kamon

copyright in each file

In kamon-jmx/src/main/scala/kamon/jmx/extension/MetricDefinition.scala
#401 (review):

  • }
    +}

+// scala sillyness
+import kamon.jmx.extension.MetricDefinition._
+
+/**

  • * This call represents one kamon metric which can be a counter, gauge,
  • * histogram or min-max counter. It allows a metric to be represented
  • * in a config file.
  • */
    +case class MetricDefinition(
  • val metricType: MetricTypeEnum.MetricType, val name: String,
  • unitOfMeasure: UnitOfMeasurement, // = UnitOfMeasurement.Unknown,
  • range: DynamicRange = null,
  • refreshInterval: FiniteDuration = null,

why you use null values instead of Options values?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#401 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ATKi9cN3R7k9GC2njpnvCF4d9zxRG8SWks5qzoqLgaJpZM4KPTwh
.

@dpsoft
Contributor
dpsoft commented Oct 14, 2016

@hunterpayne-ck we are some issues with the tests and travis..

@hunterpayne-ck
Contributor

The requested changes have been made

@hunterpayne-ck
Contributor

Ok, finally found time to make the requested changes for this PR. Nulls
are removed (except for sanity checks from outside input) and license
headers have been added.

Hunter

On Fri, Oct 14, 2016 at 7:21 AM, Diego Parra notifications@github.com
wrote:

@hunterpayne-ck https://github.com/hunterpayne-ck we are some issues
with the tests and travis..


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#401 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ATKi9f0JpxsokZsutGiVD0F339Kv2CoWks5qz5ACgaJpZM4KPTwh
.

@dpsoft
Contributor
dpsoft commented Oct 25, 2016

@hunterpayne-ck the changes are ok, but maybe is a good idea make a test. WDYT?

@hunterpayne-ck
Contributor

Ok, some tests...will be a while until I can find the time to write them. We test them at CK using an integration test against a staging monitoring stack so we haven't needed them so far. But Kamon can't do that so we probably need unit tests here.

@hunterpayne-ck
Contributor

So I've been writing a test for this PR but have run into a significant
roadblock. The BaseKamonSpec's config isn't used to initialize Kamon so I
can't activate the new JMX Exporter module that this PR introduces from
inside of the Spec class. I have another PR to allow Kamon to use a
provided config (PR 403).

#403

Perhaps we should try to get that one merged first as it makes testing much
easier? What does everyone think?

Hunter

On Tue, Oct 25, 2016 at 11:48 AM, Diego Parra notifications@github.com
wrote:

@hunterpayne-ck https://github.com/hunterpayne-ck the changes are ok,
but maybe is a good idea make a test. WDYT?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#401 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ATKi9UX8BzLD2aNTTzPXEP8qxT14eVVOks5q3k7jgaJpZM4KPTwh
.

@hunterpayne-ck
Contributor
hunterpayne-ck commented Nov 5, 2016 edited

So I got an unit test working...but its hard to have a good test suite without being able to run kamon with different configs. So this could use more testing but at least it has a basic positive unit test. Until PR #403 is merged in, doing lots of negative unit tests will be difficult. Will revisit after that happens.

@hunterpayne-ck
Contributor

Hey, we got a good build...can you merge this PR now?

@dpsoft
Contributor
dpsoft commented Nov 7, 2016

@hunterpayne-ck the PR looks good for me. I would like to merge it BUT there is one missing piece, not in this repository but in our documentation website! yes, I know, it is a bit boring to write docs but it is a necessary thing... so if you could change the jmx-documentation documentation in oder to reflect this change it would be amazing.

@hunterpayne-ck
Contributor

Can I do that in a separate PR on this branch? We have a good build and I
don't want to waste that.

Hunter

On Mon, Nov 7, 2016 at 10:49 AM, Diego Parra notifications@github.com
wrote:

@hunterpayne-ck https://github.com/hunterpayne-ck the PR looks good for
me. I would like to merge it BUT there is one missing piece, not in this
repository but in our documentation website
https://github.com/kamon-io/kamon.io! yes, I know, it is a bit boring
to write docs but it is a necessary thing... so if you could change the
jmx-documentation http://kamon.io/backends/jmx/ documentation in oder
to reflect this change it would be amazing.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#401 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ATKi9TzH92bzr0p72Vj7S4tXD6tsS4srks5q73KxgaJpZM4KPTwh
.

@dpsoft
Contributor
dpsoft commented Nov 7, 2016 edited
@hunterpayne-ck
Contributor
@dpsoft dpsoft added this to the 0.6.4 milestone Nov 8, 2016
@dpsoft dpsoft merged commit bb48b29 into kamon-io:master Nov 8, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dpsoft
Contributor
dpsoft commented Nov 8, 2016

@hunterpayne-ck thanks for this PR. Done!!!

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