-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Jolokia Module with dynamic JMX Metricset #3570
Conversation
913b10c
to
4458863
Compare
@@ -0,0 +1,4 @@ | |||
== jolokia Module | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest adding a beta[]
tag here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
return marshalJSONRequest(requestBodyStructure), responseMapping | ||
} | ||
|
||
func marshalJSONRequest(this SliceSet) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for the custom marshaling? I suspect performance is unlikely an issue here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vas78 Can you share some insights on this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No specific reason to use custom marshaling here, this was just a quick-and-dirty solution to test how it would work that eventually was left behind.
assert.Equal(t, "", jolokiaConfig[1].Instance) | ||
assert.Equal(t, "", jolokiaConfig[1].Application) | ||
assert.Equal(t, mappingConfigSpecial, jolokiaConfig[1].Mapping) | ||
assert.Equal(t, expectedBodySpecial, actualBodySpecial)*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this commented out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a left over from the "old" code base where the config had a special implementation. I removed the file now.
event[aliasStructure[0]] = map[string]interface{}{aliasStructure[1]: attibuteValue} | ||
} | ||
default: | ||
_ = fmt.Errorf("Mapping failed, alias nesting depth exceeds 1: %d", len(aliasStructure)-1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error seems completely silenced? Should it be a TODO entry? I guess the implementation shouldn't be difficult here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will try to simplify and cleanup this part further.
metricbeat/module/jolokia/jmx/jmx.go
Outdated
config := struct { | ||
Namespace string `config:"namespace" validate:"required"` | ||
Mapping []MetricSetup `config:"jmx.mappings" validate:"required"` | ||
// TODO: What are these settings for exactly? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, I wanted to complain that they are not documented :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still in progress of figuring this one out: #3051 (comment) I added the in progress
label again to the PR to make sure it does not get merged by accident.
LGTM, left some minor comments. |
2692fd8
to
0c84f51
Compare
return fmt.Errorf("No key found for metric: '%s', skipping...", metricName) | ||
} | ||
|
||
_, err := event.Put(key, attibuteValue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see why you were happy about the Put method 👍
=== Limitations | ||
No authentication against Jolokia is supported yet. | ||
No wildcards in Jolokia requests supported yet. | ||
You can get max 30 attributes from the same MBean (see Add function in config.go). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still a limitation? At list there's not Add function in config.go to check :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, this limitation was removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, happy to merge when you give the sign.
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 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
Is this consumable for Production? |
@kkmathigir It will be shipped with 6.3. If you already want to try it let me know and I can provide you a snapshot. |
Can I use it with Elasticsearch 5.6? |
@nielsen-at-cgt Yes, that should work. |
This is the implementation of a module for Jolokia which contains a dynamic jmx metricset.
An example configuration looks as following:
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:
TODO: