[activemq58] Proper prefix on service check #53
Conversation
elafarge
force-pushed
the
etienne/activemq-58-sc-hack
branch
from
7217b70 to
7218b33
Compare
| @@ -28,7 +28,7 @@ protected void sendMetricPoint(String metricName, double value, String[] tags) { | |||
| this.statsDClient.stop(); | |||
| init(); | |||
| } | |||
| statsDClient.gauge(metricName, value, tags); | |||
| statsDClient.gauge(StatsdReporter.formatDataName(metricName), value, tags); | |||
There was a problem hiding this comment.
metricName should be alias names defined in user configurations. I don't think we want/need to format it.
There was a problem hiding this comment.
Yeah, I know that. The problem is that the service check name isn't defined in user configurations :/
My point here was just to enable us to stop getting the service check prefix from the configuration file and therefore being able to have versioned configuration file without getting versioned prefixes in our backend (activemq.yaml, activemq_58.yaml, activemq_62.yaml...). In the first place I just did a dirty if(checkName == 'activemq_58') checkName = 'activemq'; do you think this would be more suitable ? Or maybe we could format the metricName when it's read from the yaml file name instead of doing it every time the metrics'service checks are sent.
There was a problem hiding this comment.
Or actually make it so the service check name is also read from the configuration, just like the metrics are... what do you think ?
|
Great work, thanks @elafarge. I wrote a few suggestions. Please let me know what you think about it. |
elafarge
force-pushed
the
etienne/activemq-58-sc-hack
branch
from
9dc8500 to
23f1ec9
Compare
I hacked the StatsDReporter to send the status of activemq.can_connect instead of activemq_58.can_connect. It's a bit better than a simple Hack. We decided that the prefix name shouldn't contain any number, underscore or capital letter (should we add the dash to this list ?). This way we can have version dependant configuration in the .yaml config files for JMXFetch without having multiple and dirty metrics and service check names in the HQ. UNIT TEST SUITE UPDATE ---------------------- Added a simple unit test to the prefix formatter, when adding versionned integration like activemq_58, it would be good to check that its prefix is correctly formatted here.
elafarge
force-pushed
the
etienne/activemq-58-sc-hack
branch
from
23f1ec9 to
789083f
Compare
|
Thanks :) I just finished implementing your suggestions. It looks better now :) |
|
Moved there #56, closing the PR. |
I hacked the StatsDReporter to send the status of activemq.can_connect
instead of activemq_58.can_connect. Hope it was the right place to do
so.