Add a generic jmx/jolokia metricset to metricbeat #3051
Conversation
|
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
|
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. |
|
@vas78 Thanks a lot for this contribution. So far we always predefined the data structure for each metricset in the fields.yml so can guarantee the mapping in elasticsearch is correct. With jmx this becomes a little bit tricky. Few questions as I'm not too familiar with the output coming from Jolokia / JMX:
|
|
@ruflin I'm afraid that's exacly the point: it's up to beats user to decide which metrics he/she would like to collect with jolokia. I could image adding some common fields like uptime, heap size, java process cpu usage etc, but I`m not quite sure if it makes any sense here. To the second question: I could try converting the current solution with an embedded json in mapping into an array of values like "metric:alias", e.g.: What do you think? |
|
@vas78 Sorry for the late answer. Some more thoughts form my side: We recently introduce Looking at your events above it seems like that data for a specific MBean is known in advance. So what if we would split the above up in 2 parts:
In the above case we would have for example: In your test cases you also have Cassandra data. So the The customer mapping definition of the jmx module goes in a similar direction as the request here: #2987 There are still lots of discussion ongoing on how to make this happen best in a generic way. Inputs are more then welcome. @vas78 Does the above assumptions make sense related to JMX? @radoondas Mentioning you here as I know you were also looking in JMX in the past. |
|
@ruflin Ok, makes sense. I will split up the module into some "standard" metricsets like memory, runtime, etc. and one "free-style" metricset that can be configured as desired. |
|
@vas78 Great. Potentially you can split it up in 2 PR's because we will potentially have some additional discussions on the |
|
@vas78 Anything I could help here with? It would be really nice to get support for jolokia / jmx in metricbeat. |
|
@ruflin sorry, it`s taking a bit longer than I though especially due to all the deadlines just before holidays :( I will provide an update soon. |
|
@vas78 No worries. Looking forward to the changes. |
|
@vas78 Did you have any chance to work on this one recently? |
|
@ruflin I`ve made two attempts to refactor the code and split up the module into "standard" and "custom" part, but did not really like the result. Yes, there are some metrics in MBean that are always present in every JVM, but one of the most important of these "standard" metrics - GC - depends heavily on the GC strategy configured in the application. Hard coding all of these strategies makes not that much sense since we already have a dynamic metrics mapping that solves this problem. The rest of these "standard" metrics are just some CPU and threading usage stats that are not that valuable alone in this context. Your proposal regarding the possible future use case for custom jolokia module is embedding it into other application specific modules, e.g. cassandra, kafka, etc. and hence providing a strict mapping for each application metric set. I still think it`ll be pretty hard to guess which metrics would the "common" user pick for each application and this could end up in the constant flow of requests to add this one and that one extra metric. So why not let the users decide and configure it for themselves from the very beginning? The only difficulty in that case is really just creating/adjusting the right index mapping in Elastic which is quite simple in that case (I had to do that multiple times daily in the very beginning for every application we currently monitor with beats). Having said all that, I would like to return to the original PR and focus on the dynamic jolokia module, rather than creating new static metricsets. Maybe I could in fact simplify the config and make it more user-friendly? Please let me know what you think. |
|
@vas78 Thanks for pushing forward on this one. The reason for the fixed mapping is that we try to have a good out of the box experience whenever possible. But I see your point that with jolokia this can become quite tricky. So 👍 on moving forward with the initial proposal and figure out some better options for how to configure it. In the meantime we also have the prometheus/collector metricset wich builds the foundation of such a "dynamic" metricset: https://github.com/elastic/beats/tree/master/metricbeat/module/prometheus/collector The most important part here is that it can set the namespace: https://github.com/elastic/beats/blob/master/metricbeat/module/prometheus/collector/collector.go#L107 Could you update your PR with making the namespace possible? I will have a look again on your PR with the above in mind. |
|
I looked at the configuration again and here are some ideas on what we could do: JSON based: One line per metric: Abstracted to YAML: The above would allow, to potentially add the field type which would allow us to create a template out of it. So we could potentially create a template on the first run and push it to elasticsearch. My favorite option is currently the Abstract YAML one because I think it is the most extentable and it does not require the user to get the strings correctly, like 3 |
|
@nfrankel Would be interesting to get your input on this one as you recently published https://blog.frankel.ch/feedback-on-feeding-spring-boot-metrics-to-elasticsearch/#gsc.tab=0 Which way would work best for you? |
| func marshalJSONRequest(this SliceSet) string { | ||
| result := "[" | ||
| for mbean, attributes := range this { | ||
| singleRequest := "{\"type\":\"read\",\"mbean\":\"" + mbean + "\",\"attribute\":[" |
There was a problem hiding this comment.
You should be careful when taking inserting the MBean's ObjectName as it can contain all kind of weired characters, including quotes which would make this request illegal json. You would need to escape included quotes with \
Attribute names are much more regular, but still quotes are allowed according to the JMX specs, so it wouldn't harm to quote them, too.
|
@ruflin Actually I liked the idea of the abstracted yaml since it eliminates the necessity to use some custom delimiter for mbean and attribute values that the "one line per metric" approach requires. And since we will still need not just the mbean object name itself but the attribute and it`s mapping as well, what would you say to this one?
This would allow lazy configurers like @nfrankel (and myself :)) to copy-paste the mbean name from jconsole on the one hand and group all attributes belonging to the the same mbean and thus improve config readability and maintainability on the other. |
|
@vas78 SGTM. You guys are here the experts :-) |
| @@ -0,0 +1,6 @@ | |||
| - module: jmx | |||
There was a problem hiding this comment.
We should call the module jolokia (as this is the service we connect to) and the namespace will be dynamically defined as part of the config.
| @@ -0,0 +1,6 @@ | |||
| - module: jmx | |||
| metricsets: ["jolokia"] | |||
There was a problem hiding this comment.
We could call the "dynamic" metricset also collector as we do in prometheus. Or do you have some better ideas here?
|
@vas78 It seems some generate files are out of date. Try to run Can you ping me when I should have a closer look again? |
|
@ruflin Renamed the module and ran make update, but the "check" CI Target still fails, can you please take a look at it? Thanks! |
|
@vas78 For the For the other test failing. This could be old flaky one. Any chance that you could rebase on master? This should also resolved the conflicts above (files were removed in a recent PR). |
| @@ -121,6 +121,14 @@ metricbeat.modules: | |||
| #period: 10s | |||
| #hosts: ["tcp://127.0.0.1:14567"] | |||
|
|
|||
| #------------------------------- Jolokia Module ------------------------------ | |||
| - module: jolokia | |||
| metricsets: ["dynamic"] | |||
There was a problem hiding this comment.
should we call it jmx? Not sure about the name :-)
There was a problem hiding this comment.
Ignore that and lets go for dynamic at the moment (except you like jmx better).
| enabled: true | ||
| period: 10s | ||
| hosts: ["localhost"] | ||
| namespace: "jolokia_metrics" |
There was a problem hiding this comment.
lets call this metrics as jolokia will be already part of the namespace anyways.
|
|
||
|
|
||
| [float] | ||
| === jolokia.dynamic._namespace |
There was a problem hiding this comment.
No need to document this one. This field will be removed and placed under metricset.namespace.
|
|
||
| Namespace | ||
|
|
||
| [float] |
There was a problem hiding this comment.
Do these 2 fields really exist in all events?
| @@ -46,7 +46,13 @@ metricbeat.modules: | |||
| period: 10s | |||
| processes: ['.*'] | |||
|
|
|||
|
|
|||
| #------------------------------- Jolokia Module ------------------------------ | |||
There was a problem hiding this comment.
If you use short_config: falsein the module fields.yml it will not show up here. See https://github.com/elastic/beats/blob/master/metricbeat/module/kafka/_meta/fields.yml#L7
| @@ -0,0 +1,7 @@ | |||
| - module: jolokia | |||
There was a problem hiding this comment.
Currently we commend out the config by default so it is not enabled. This might change in the near future: https://github.com/elastic/beats/blob/master/metricbeat/module/kafka/_meta/config.yml
| }, | ||
| "jolokia": { | ||
| "dynamic": { | ||
| "gc": { |
There was a problem hiding this comment.
This needs to be updated / removed to not cause confusion.
| event := map[string]interface{}{} | ||
|
|
||
| if application != "" { | ||
| event["application"] = application |
There was a problem hiding this comment.
I see, this is really a predefined field. Except for putting it under a fixed namespace I don't have a good idea yet where we could put these and how to document it.
In docker we have a similar problem where several metricsets also have the container info. We use some tricks there to add this info the the evend and have it then under the container metricset. Perhaps we should also introduce such a namespace here. jolokia.?.application. Like this we can also document it as it will not be under the dynamic namespace. Some suggestions with what we should replace ?
| title: "Jolokia" | ||
| description: > | ||
| Jolokia Module | ||
| fields: |
There was a problem hiding this comment.
Can you add an experimental flag here? https://github.com/elastic/beats/blob/master/metricbeat/module/kafka/_meta/fields.yml#L6
| // New create a new instance of the MetricSet | ||
| func New(base mb.BaseMetricSet) (mb.MetricSet, error) { | ||
| // Additional configuration options | ||
| config := struct { |
There was a problem hiding this comment.
Please add an experimental message here: https://github.com/elastic/beats/blob/master/metricbeat/module/kafka/partition/partition.go#L40
|
@vas78 I was thinking if we can perhaps help you with this PR to get it into metricbeat? Let me know if we should take over from here or if we can help you in any way. |
|
@ruflin Sorry, I was totally overloaded with some other projects lately and had no time to look into this one. Would be great if you can take it over from here, many thanks! |
|
@vas78 Great, will take over from here and ping you in case I have some questions :-) |
|
@vas78 I'm currently work on the jmx module and discovered the following. We have Do I miss something here that requires the additional |
|
@ruflin You got it all right, there's no other reason to have |
|
@vas78 Thanks. An other question: I'm trying to get my head around the application and instance config and fields. What are these exactly used for? Could these also just go under |
|
@ruflin These config items were useful in the first implementation with multiple hosts under |
|
Thanks for the feedback. Will remove the fields then. |
This is the implementation of a module for Jolokia which contains a dynamic jmx metricset.
An example configuration looks as following:
```
- module: jolokia
metricsets: ["jmx"]
enabled: true
period: 1s
hosts: ["localhost:8778"]
namespace: "metrics"
jmx.mappings:
- mbean: 'java.lang:type=Runtime'
attributes:
- attr: Uptime
field: uptime
- mbean: 'java.lang:type=GarbageCollector,name=ConcurrentMarkSweep'
attributes:
- attr: CollectionTime
field: gc.cms_collection_time
- attr: CollectionCount
field: gc.cms_collection_count
- mbean: 'java.lang:type=Memory'
attributes:
- attr: HeapMemoryUsage
field: memory.heap_usage
- attr: NonHeapMemoryUsage
field: memory.non_heap_usage
```
For each mbeat the attributes which should be fetched can be defined. The field defines under which field name the event will be put. The namespace defines the metricset namespace.
This PR replaces elastic#3051
Further changes:
* Added support for method and body to http helper
* Handle empty fields in generators. This happens for a module which only contains dynamic metricsets which is currently the case for jolokia.
TODO:
* [x] Add system tests
* [x] Check documentation
* [x] Add integration test
* [ ] Open issue for metricset which contains basic memory info
This is the implementation of a module for Jolokia which contains a dynamic jmx metricset.
An example configuration looks as following:
```
- module: jolokia
metricsets: ["jmx"]
enabled: true
period: 1s
hosts: ["localhost:8778"]
namespace: "metrics"
jmx.mappings:
- mbean: 'java.lang:type=Runtime'
attributes:
- attr: Uptime
field: uptime
- mbean: 'java.lang:type=GarbageCollector,name=ConcurrentMarkSweep'
attributes:
- attr: CollectionTime
field: gc.cms_collection_time
- attr: CollectionCount
field: gc.cms_collection_count
- mbean: 'java.lang:type=Memory'
attributes:
- attr: HeapMemoryUsage
field: memory.heap_usage
- attr: NonHeapMemoryUsage
field: memory.non_heap_usage
```
For each mbeat the attributes which should be fetched can be defined. The field defines under which field name the event will be put. The namespace defines the metricset namespace.
This PR replaces #3051
Further changes:
* Added support for method and body to http helper
* Handle empty fields in generators. This happens for a module which only contains dynamic metricsets which is currently the case for jolokia.
TODO:
* [x] Add system tests
* [x] Check documentation
* [x] Add integration test
* [ ] Open issue for metricset which contains basic memory info
|
@vas78 @nfrankel @rhuss As discussed previously, as a next step I would also like to have some structured metricsets for Jolokia. For this I opened the issue here: #3585 But I will need your help with this one as I need your insights into which metrics exists for most installation. If you could provide me with some configurations for the jmx metricset that should become a Metricset on its own I would be more then happy to implement them. Lets continue the discussion in #3585 |
This is the implementation of a module for Jolokia which contains a dynamic jmx metricset.
An example configuration looks as following:
```
- module: jolokia
metricsets: ["jmx"]
enabled: true
period: 1s
hosts: ["localhost:8778"]
namespace: "metrics"
jmx.mappings:
- mbean: 'java.lang:type=Runtime'
attributes:
- attr: Uptime
field: uptime
- mbean: 'java.lang:type=GarbageCollector,name=ConcurrentMarkSweep'
attributes:
- attr: CollectionTime
field: gc.cms_collection_time
- attr: CollectionCount
field: gc.cms_collection_count
- mbean: 'java.lang:type=Memory'
attributes:
- attr: HeapMemoryUsage
field: memory.heap_usage
- attr: NonHeapMemoryUsage
field: memory.non_heap_usage
```
For each mbeat the attributes which should be fetched can be defined. The field defines under which field name the event will be put. The namespace defines the metricset namespace.
This PR replaces elastic#3051
Further changes:
* Added support for method and body to http helper
* Handle empty fields in generators. This happens for a module which only contains dynamic metricsets which is currently the case for jolokia.
TODO:
* [x] Add system tests
* [x] Check documentation
* [x] Add integration test
* [ ] Open issue for metricset which contains basic memory info
This is a generic metricset for fetching data from jmx using jolokia.