Generic jmx importing #401

hunterpayne-ck · 2016-10-05T21:28:03Z

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.

… custom JMX properties into Kamon

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?

dpsoft · 2016-10-13T19:41:07Z

kamon-jmx/src/main/scala/kamon/jmx/extension/ExportedMBeanQuery.scala

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


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

dpsoft · 2016-10-13T19:42:44Z

kamon-jmx/src/main/scala/kamon/jmx/extension/MetricDefinition.scala

+    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?

hunterpayne-ck · 2016-10-13T21:38:33Z

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 · 2016-10-14T14:21:51Z

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

…requested

hunterpayne-ck · 2016-10-22T00:14:43Z

The requested changes have been made

hunterpayne-ck · 2016-10-25T18:45:03Z

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 · 2016-10-25T18:48:00Z

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

…build in travis

hunterpayne-ck · 2016-10-25T20:58:46Z

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 · 2016-11-02T23:54:34Z

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
.

…c conversions in the code

hunterpayne-ck · 2016-11-05T00:55:12Z

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 · 2016-11-07T18:31:16Z

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

dpsoft · 2016-11-07T18:49:16Z

@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 · 2016-11-07T18:59:37Z

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 · 2016-11-07T19:00:58Z

@hunterpayne-ck please make a new PR here: https://github.com/kamon-io/kamon.io/blob/master/backends/jmx.md

hunterpayne-ck · 2016-11-07T21:55:58Z

Done...kamon-io/kamon.io#27

dpsoft · 2016-11-08T00:29:27Z

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

Hunter Payne added 3 commits October 5, 2016 14:23

generic-jmx-importing: adding classes that make it possible to import…

06c5670

… custom JMX properties into Kamon

generic-jmx-importing: adding reference resources for new jmx importer

42e6128

generic-jmx-importing: adding a missing return to end of file

85ffba8

dpsoft reviewed Oct 13, 2016

View reviewed changes

generic-jmx-importing: adding copywrite header and removing nulls as …

f7cfb35

…requested

generic-jmx-importing: triggering another build to try to get a good …

b8e3b8b

…build in travis

generic-jmx-importing: adding a test and fixing problems with numberi…

0f48c2a

…c conversions in the code

dpsoft added this to the 0.6.4 milestone Nov 8, 2016

dpsoft merged commit bb48b29 into kamon-io:master Nov 8, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generic jmx importing #401

Generic jmx importing #401

hunterpayne-ck commented Oct 5, 2016

dpsoft left a comment

dpsoft Oct 13, 2016

dpsoft Oct 13, 2016

hunterpayne-ck commented Oct 13, 2016

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

copyright in each file

dpsoft commented Oct 14, 2016

hunterpayne-ck commented Oct 22, 2016

hunterpayne-ck commented Oct 25, 2016

dpsoft commented Oct 25, 2016

hunterpayne-ck commented Oct 25, 2016

hunterpayne-ck commented Nov 2, 2016

hunterpayne-ck commented Nov 5, 2016 •

edited

hunterpayne-ck commented Nov 7, 2016

dpsoft commented Nov 7, 2016

hunterpayne-ck commented Nov 7, 2016

dpsoft commented Nov 7, 2016 •

edited

hunterpayne-ck commented Nov 7, 2016

dpsoft commented Nov 8, 2016

Generic jmx importing #401

Generic jmx importing #401

Conversation

hunterpayne-ck commented Oct 5, 2016

dpsoft left a comment

Choose a reason for hiding this comment

dpsoft Oct 13, 2016

Choose a reason for hiding this comment

dpsoft Oct 13, 2016

Choose a reason for hiding this comment

hunterpayne-ck commented Oct 13, 2016

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

copyright in each file

dpsoft commented Oct 14, 2016

hunterpayne-ck commented Oct 22, 2016

hunterpayne-ck commented Oct 25, 2016

dpsoft commented Oct 25, 2016

hunterpayne-ck commented Oct 25, 2016

hunterpayne-ck commented Nov 2, 2016

hunterpayne-ck commented Nov 5, 2016 • edited

hunterpayne-ck commented Nov 7, 2016

dpsoft commented Nov 7, 2016

hunterpayne-ck commented Nov 7, 2016

dpsoft commented Nov 7, 2016 • edited

hunterpayne-ck commented Nov 7, 2016

dpsoft commented Nov 8, 2016

hunterpayne-ck commented Nov 5, 2016 •

edited

dpsoft commented Nov 7, 2016 •

edited