From 5e98ce55c28668db634bcafd42017816a6a98644 Mon Sep 17 00:00:00 2001 From: Jaime Soriano Pastor Date: Wed, 13 Jun 2018 19:28:35 +0200 Subject: [PATCH] Metricbeat: Canonicalize MBean names in Jolokia module We were setting the `canonicalNaming` option to `false` in requests to Jolokia, assuming that it should leave MBean names in responses as they are. We need that for mappings to work. But in some scenarios, as when using wildcards, Jolokia is responding with canonicalized names, what breaks mappings. This change canonicalizes names from config only for mappings and the pre-built request. If we see that after setting `canonicalNaming` to `true` there is some scenario in which Jolokia replies with non-canonicalized names, then we'd have to canonicalize also MBeans for responses, or rethink how we do the mapping. --- CHANGELOG.asciidoc | 1 + metricbeat/module/jolokia/jmx/config.go | 57 +++++++++++++- metricbeat/module/jolokia/jmx/config_test.go | 81 ++++++++++++++++++++ metricbeat/module/jolokia/jmx/data.go | 6 +- metricbeat/module/jolokia/jmx/jmx.go | 1 - metricbeat/tests/system/test_jolokia.py | 4 +- 6 files changed, 143 insertions(+), 7 deletions(-) create mode 100644 metricbeat/module/jolokia/jmx/config_test.go diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index fe963a6ea30c..1a2e98e81874 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -114,6 +114,7 @@ https://github.com/elastic/beats/compare/v6.2.3...master[Check the HEAD diff] - Ensure canonical naming for JMX beans is disabled in Jolokia module. {pull}7047[7047] - Fix field mapping for the system process CPU ticks fields. {pull}7230[7230] - Fix Windows service metricset when using a 32-bit binary on a 64-bit OS. {pull}7294[7294] +- Fix Jolokia attribute mapping when using wildcards and MBean names with multiple properties. {pull}7321[7321] *Packetbeat* diff --git a/metricbeat/module/jolokia/jmx/config.go b/metricbeat/module/jolokia/jmx/config.go index 56eb6d659917..8bc4bcf07973 100644 --- a/metricbeat/module/jolokia/jmx/config.go +++ b/metricbeat/module/jolokia/jmx/config.go @@ -1,6 +1,12 @@ package jmx -import "encoding/json" +import ( + "encoding/json" + "fmt" + "regexp" + "sort" + "strings" +) type JMXMapping struct { MBean string @@ -80,18 +86,61 @@ func (m AttributeMapping) Get(mbean, attr string) (Attribute, bool) { return a, found } +// Parse strings with properties with the format key=value, being: +// - key a nonempty string of characters which may not contain any of the characters, +// comma (,), equals (=), colon, asterisk, or question mark. +// - value a string that can be quoted or unquoted, if unquoted it cannot be empty and +// cannot contain any of the characters comma, equals, colon, or quote. +var propertyRegexp = regexp.MustCompile("[^,=:*?]+=([^,=:\"]+|\".*\")") + +func canonicalizeMBeanName(name string) (string, error) { + // From https://docs.oracle.com/javase/8/docs/api/javax/management/ObjectName.html#getCanonicalName-- + // + // Returns the canonical form of the name; that is, a string representation where the + // properties are sorted in lexical order. + // The canonical form of the name is a String consisting of the domain part, + // a colon (:), the canonical key property list, and a pattern indication. + // + parts := strings.SplitN(name, ":", 2) + if len(parts) != 2 || parts[0] == "" || parts[1] == "" { + return name, fmt.Errorf("domain and properties needed in mbean name: %s", name) + } + domain := parts[0] + + // Using this regexp instead of just splitting by commas because values can be quoted + // and contain commas, what complicates the parsing. + properties := propertyRegexp.FindAllString(parts[1], -1) + propertyList := strings.Join(properties, ",") + if len(propertyList) != len(parts[1]) { + // Some property didn't match + return name, fmt.Errorf("mbean properties must be in the form key=value: %s", name) + } + + sort.Strings(properties) + return domain + ":" + strings.Join(properties, ","), nil +} + func buildRequestBodyAndMapping(mappings []JMXMapping) ([]byte, AttributeMapping, error) { responseMapping := make(AttributeMapping) var blocks []RequestBlock + // At least Jolokia 1.5 responses with canonicalized MBean names when using + // wildcards, even when canonicalNaming is set to false, this makes mappings to fail. + // So use canonicalzed names everywhere. + // If Jolokia returns non-canonicalized MBean names, then we'll need to canonicalize + // them or change our approach to mappings. config := map[string]interface{}{ "ignoreErrors": true, - "canonicalNaming": false, + "canonicalNaming": true, } for _, mapping := range mappings { + mbean, err := canonicalizeMBeanName(mapping.MBean) + if err != nil { + return nil, nil, err + } rb := RequestBlock{ Type: "read", - MBean: mapping.MBean, + MBean: mbean, Config: config, } @@ -104,7 +153,7 @@ func buildRequestBodyAndMapping(mappings []JMXMapping) ([]byte, AttributeMapping for _, attribute := range mapping.Attributes { rb.Attribute = append(rb.Attribute, attribute.Attr) - responseMapping[attributeMappingKey{mapping.MBean, attribute.Attr}] = attribute + responseMapping[attributeMappingKey{mbean, attribute.Attr}] = attribute } blocks = append(blocks, rb) } diff --git a/metricbeat/module/jolokia/jmx/config_test.go b/metricbeat/module/jolokia/jmx/config_test.go new file mode 100644 index 000000000000..4e1f3f210cd5 --- /dev/null +++ b/metricbeat/module/jolokia/jmx/config_test.go @@ -0,0 +1,81 @@ +package jmx + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestCanonicalMBeanName(t *testing.T) { + cases := []struct { + mbean string + expected string + ok bool + }{ + { + mbean: ``, + ok: false, + }, + { + mbean: `type=Runtime`, + ok: false, + }, + { + mbean: `java.lang`, + ok: false, + }, + { + mbean: `java.lang:`, + ok: false, + }, + { + mbean: `java.lang:type=Runtime,name`, + ok: false, + }, + { + mbean: `java.lang:type=Runtime`, + expected: `java.lang:type=Runtime`, + ok: true, + }, + { + mbean: `java.lang:name=Foo,type=Runtime`, + expected: `java.lang:name=Foo,type=Runtime`, + ok: true, + }, + { + mbean: `java.lang:type=Runtime,name=Foo`, + expected: `java.lang:name=Foo,type=Runtime`, + ok: true, + }, + { + mbean: `java.lang:type=Runtime,name=Foo*`, + expected: `java.lang:name=Foo*,type=Runtime`, + ok: true, + }, + { + mbean: `java.lang:type=Runtime,name=*`, + expected: `java.lang:name=*,type=Runtime`, + ok: true, + }, + { + mbean: `java.lang:type=Runtime,name="foo,bar"`, + expected: `java.lang:name="foo,bar",type=Runtime`, + ok: true, + }, + { + mbean: `Catalina:type=RequestProcessor,worker="http-nio-8080",name=HttpRequest1`, + expected: `Catalina:name=HttpRequest1,type=RequestProcessor,worker="http-nio-8080"`, + ok: true, + }, + } + + for _, c := range cases { + canonical, err := canonicalizeMBeanName(c.mbean) + if c.ok { + assert.NoError(t, err, "failed parsing for: "+c.mbean) + assert.Equal(t, c.expected, canonical, "mbean: "+c.mbean) + } else { + assert.Error(t, err, "should have failed for: "+c.mbean) + } + } +} diff --git a/metricbeat/module/jolokia/jmx/data.go b/metricbeat/module/jolokia/jmx/data.go index 8088acfccd7f..61ea8e36a98b 100644 --- a/metricbeat/module/jolokia/jmx/data.go +++ b/metricbeat/module/jolokia/jmx/data.go @@ -8,6 +8,7 @@ import ( "github.com/pkg/errors" "github.com/elastic/beats/libbeat/common" + "github.com/elastic/beats/libbeat/logp" ) const ( @@ -149,7 +150,10 @@ func parseResponseEntry( ) error { field, exists := mapping.Get(requestMbeanName, attributeName) if !exists { - return errors.Errorf("metric key '%v' for mbean '%s' not found in mapping (%+v)", attributeName, requestMbeanName, mapping) + // This shouldn't ever happen, if it does it is probably that some of our + // assumptions when building the request and the mapping is wrong. + logp.Debug("jolokia.jmx", "mapping: %+v", mapping) + return errors.Errorf("metric key '%v' for mbean '%s' not found in mapping", attributeName, requestMbeanName) } var key eventKey diff --git a/metricbeat/module/jolokia/jmx/jmx.go b/metricbeat/module/jolokia/jmx/jmx.go index 5b84d9876076..10a502fdeef3 100644 --- a/metricbeat/module/jolokia/jmx/jmx.go +++ b/metricbeat/module/jolokia/jmx/jmx.go @@ -45,7 +45,6 @@ type MetricSet struct { // New create a new instance of the MetricSet func New(base mb.BaseMetricSet) (mb.MetricSet, error) { - config := struct { Namespace string `config:"namespace" validate:"required"` Mappings []JMXMapping `config:"jmx.mappings" validate:"required"` diff --git a/metricbeat/tests/system/test_jolokia.py b/metricbeat/tests/system/test_jolokia.py index 64c9f42742db..921ee5aef254 100644 --- a/metricbeat/tests/system/test_jolokia.py +++ b/metricbeat/tests/system/test_jolokia.py @@ -11,7 +11,9 @@ class Test(metricbeat.BaseTest): @parameterized.expand([ 'java.lang:name=PS MarkSweep,type=GarbageCollector', - 'java.lang:type=GarbageCollector,name=PS MarkSweep' + 'java.lang:type=GarbageCollector,name=PS MarkSweep', + 'java.lang:name=*,type=GarbageCollector', + 'java.lang:type=GarbageCollector,name=*', ]) @unittest.skipUnless(metricbeat.INTEGRATION_TESTS, "integration test") def test_jmx(self, mbean):