Metricbeat: Canonicalize MBean names in Jolokia module (elastic#7321)

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

@@ -115,6 +115,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*
 

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)
 	}

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)
+		}
+	}
+}

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

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"`

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):