This is an automated email from the ASF dual-hosted git repository.
rombert pushed a commit to branch master
in repository
https://gitbox.apache.org/repos/asf/sling-org-apache-sling-commons-metrics.git
commit dab007438844003c68e2ea80c15bbf4dd797127c
Author: Chetan Mehrotra <chet...@apache.org>
AuthorDate: Thu Nov 17 05:08:41 2016 +0000
SLING-5443 - Review the naming conventions used by JMXReporter to register
the Metric MBeans
Specify type for JMX ObjectNames created for Metrics
git-svn-id: https://svn.apache.org/repos/asf/sling/trunk@1770120
13f79535-47bb-0310-9956-ffa450edef68
---
.../metrics/internal/BundleMetricsMapper.java | 31 ++++----
.../sling/commons/metrics/internal/JmxUtil.java | 89 ++++++++++++++++++++++
.../metrics/internal/BundleMetricsMapperTest.java | 7 --
.../commons/metrics/internal/JmxUtilTest.java | 51 +++++++++++++
.../metrics/internal/MetricServiceTest.java | 3 +-
5 files changed, 156 insertions(+), 25 deletions(-)
diff --git
a/src/main/java/org/apache/sling/commons/metrics/internal/BundleMetricsMapper.java
b/src/main/java/org/apache/sling/commons/metrics/internal/BundleMetricsMapper.java
index 9a74a1e..8bdbd45 100644
---
a/src/main/java/org/apache/sling/commons/metrics/internal/BundleMetricsMapper.java
+++
b/src/main/java/org/apache/sling/commons/metrics/internal/BundleMetricsMapper.java
@@ -19,10 +19,12 @@
package org.apache.sling.commons.metrics.internal;
+import java.util.Hashtable;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
+import javax.management.MalformedObjectNameException;
import javax.management.ObjectName;
import com.codahale.metrics.DefaultObjectNameFactory;
@@ -35,6 +37,7 @@ import org.slf4j.LoggerFactory;
class BundleMetricsMapper implements ObjectNameFactory{
public static final String HEADER_DOMAIN_NAME = "Sling-Metrics-Domain";
public static final String DEFAULT_DOMAIN_NAME = "org.apache.sling";
+ static final String JMX_TYPE_METRICS = "Metrics";
private final Logger log = LoggerFactory.getLogger(getClass());
private final ConcurrentMap<String, Bundle> metricToBundleMapping = new
ConcurrentHashMap<>();
private final MetricRegistry registry;
@@ -58,11 +61,20 @@ class BundleMetricsMapper implements ObjectNameFactory{
@Override
public ObjectName createName(String type, String domain, String name) {
- String mappedDomainName = safeDomainName(getDomainName(name));
+ String mappedDomainName = JmxUtil.safeDomainName(getDomainName(name));
if (mappedDomainName == null) {
mappedDomainName = domain;
}
- return defaultFactory.createName(type, mappedDomainName, name);
+
+ Hashtable<String, String> table = new Hashtable<String, String>();
+ table.put("type", JMX_TYPE_METRICS);
+ table.put("name", JmxUtil.quoteValueIfRequired(name));
+ try {
+ return new ObjectName(mappedDomainName, table);
+ } catch (MalformedObjectNameException e) {
+ log.warn("Unable to register {} {}", type, name, e);
+ throw new RuntimeException(e);
+ }
}
private String getDomainName(String name) {
@@ -84,19 +96,4 @@ class BundleMetricsMapper implements ObjectNameFactory{
return bundle.getSymbolicName();
}
- static String safeDomainName(String name){
- if (name == null){
- return null;
- }
-
- name = name.trim();
-
- //Taken from javax.management.ObjectName.isDomain()
- //Following are special chars in domain name
- name = name.replace(':', '_');
- name = name.replace('*', '_');
- name = name.replace('?', '_');
- name = name.replace('\n', '_');
- return name;
- }
}
diff --git
a/src/main/java/org/apache/sling/commons/metrics/internal/JmxUtil.java
b/src/main/java/org/apache/sling/commons/metrics/internal/JmxUtil.java
new file mode 100644
index 0000000..9938631
--- /dev/null
+++ b/src/main/java/org/apache/sling/commons/metrics/internal/JmxUtil.java
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.sling.commons.metrics.internal;
+
+import javax.management.MalformedObjectNameException;
+import javax.management.ObjectName;
+
+/**
+ * Utility methods related to JMX
+ */
+final class JmxUtil {
+
+ /**
+ * Checks if the passed value string can be used as is as part of
+ * JMX {@link javax.management.ObjectName} If it cannot be used then
+ * it would return a quoted string which is then safe to be used
+ * as part of ObjectName.
+ *
+ * <p>This is meant to avoid unnecessary quoting of value</p>
+ *
+ * @param unquotedValue to quote if required
+ * @return passed value or quoted value if required
+ */
+ public static String quoteValueIfRequired(String unquotedValue) {
+ String result;
+ String quotedValue = ObjectName.quote(unquotedValue);
+
+ //Check if some chars are escaped or not. In that case
+ //length of quoted string (excluding quotes) would differ
+ if (quotedValue.substring(1, quotedValue.length() -
1).equals(unquotedValue)) {
+ ObjectName on = null;
+ try {
+ //Quoting logic in ObjectName does not escape ',', '='
+ //etc. So try now by constructing ObjectName. If that
+ //passes then value can be used as safely
+
+ //Also we cannot just rely on ObjectName as it treats
+ //*, ? as pattern chars and which should ideally be escaped
+ on = new ObjectName("dummy", "dummy", unquotedValue);
+ } catch (MalformedObjectNameException ignore) {
+ //ignore
+ }
+
+ if (on != null){
+ result = unquotedValue;
+ } else {
+ result = quotedValue;
+ }
+ } else {
+ //Some escaping done. So do quote
+ result = quotedValue;
+ }
+ return result;
+ }
+
+
+ public static String safeDomainName(String name){
+ if (name == null){
+ return null;
+ }
+
+ name = name.trim();
+
+ //Taken from javax.management.ObjectName.isDomain()
+ //Following are special chars in domain name
+ name = name.replace(':', '_');
+ name = name.replace('*', '_');
+ name = name.replace('?', '_');
+ name = name.replace('\n', '_');
+ return name;
+ }
+}
diff --git
a/src/test/java/org/apache/sling/commons/metrics/internal/BundleMetricsMapperTest.java
b/src/test/java/org/apache/sling/commons/metrics/internal/BundleMetricsMapperTest.java
index d34357a..ae08a05 100644
---
a/src/test/java/org/apache/sling/commons/metrics/internal/BundleMetricsMapperTest.java
+++
b/src/test/java/org/apache/sling/commons/metrics/internal/BundleMetricsMapperTest.java
@@ -66,12 +66,5 @@ public class BundleMetricsMapperTest {
assertEquals("com.test", name.getDomain());
}
- @Test
- public void safeDomainName() throws Exception{
- assertEquals("com.foo", BundleMetricsMapper.safeDomainName("com.foo"));
- assertEquals("com_foo", BundleMetricsMapper.safeDomainName("com:foo"));
- assertEquals("com_foo", BundleMetricsMapper.safeDomainName("com?foo"));
- assertEquals("com_foo", BundleMetricsMapper.safeDomainName("com*foo"));
- }
}
\ No newline at end of file
diff --git
a/src/test/java/org/apache/sling/commons/metrics/internal/JmxUtilTest.java
b/src/test/java/org/apache/sling/commons/metrics/internal/JmxUtilTest.java
new file mode 100644
index 0000000..9db25c7
--- /dev/null
+++ b/src/test/java/org/apache/sling/commons/metrics/internal/JmxUtilTest.java
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.sling.commons.metrics.internal;
+
+import junit.framework.TestCase;
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+public class JmxUtilTest {
+
+ @Test
+ public void quotation() throws Exception{
+ assertEquals("text", JmxUtil.quoteValueIfRequired("text"));
+ TestCase.assertEquals("", JmxUtil.quoteValueIfRequired(""));
+
assertTrue(JmxUtil.quoteValueIfRequired("text*with?chars").startsWith("\""));
+ }
+
+ @Test
+ public void quoteAndComma() throws Exception{
+
assertTrue(JmxUtil.quoteValueIfRequired("text,withComma").startsWith("\""));
+
assertTrue(JmxUtil.quoteValueIfRequired("text=withEqual").startsWith("\""));
+ }
+
+ @Test
+ public void safeDomainName() throws Exception{
+ assertEquals("com.foo", JmxUtil.safeDomainName("com.foo"));
+ assertEquals("com_foo", JmxUtil.safeDomainName("com:foo"));
+ assertEquals("com_foo", JmxUtil.safeDomainName("com?foo"));
+ assertEquals("com_foo", JmxUtil.safeDomainName("com*foo"));
+ }
+
+}
\ No newline at end of file
diff --git
a/src/test/java/org/apache/sling/commons/metrics/internal/MetricServiceTest.java
b/src/test/java/org/apache/sling/commons/metrics/internal/MetricServiceTest.java
index ca76690..72a8e02 100644
---
a/src/test/java/org/apache/sling/commons/metrics/internal/MetricServiceTest.java
+++
b/src/test/java/org/apache/sling/commons/metrics/internal/MetricServiceTest.java
@@ -41,6 +41,7 @@ import org.junit.After;
import org.junit.Rule;
import org.junit.Test;
+import static
org.apache.sling.commons.metrics.internal.BundleMetricsMapper.JMX_TYPE_METRICS;
import static org.hamcrest.Matchers.empty;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;
@@ -134,7 +135,7 @@ public class MetricServiceTest {
Meter meter = service.meter("test");
assertNotNull(meter);
QueryExp q =
Query.isInstanceOf(Query.value(JmxReporter.JmxMeterMBean.class.getName()));
- Set<ObjectName> names = server.queryNames(new
ObjectName("org.apache.sling:name=*"), q);
+ Set<ObjectName> names = server.queryNames(new
ObjectName("org.apache.sling:name=*,type="+ JMX_TYPE_METRICS), q);
assertThat(names, is(not(empty())));
MockOsgi.deactivate(service, context.bundleContext());
--
To stop receiving notification emails like this one, please contact
"commits@sling.apache.org" <commits@sling.apache.org>.
