SLING-7074 - RRD4J improve configuration handling

Submitted-By: Alex Deparvu

git-svn-id: https://svn.apache.org/repos/asf/sling/trunk@1806051 13f79535-47bb-0310-9956-ffa450edef68
diff --git a/src/main/java/org/apache/sling/commons/metrics/rrd4j/impl/CodahaleMetricsReporter.java b/src/main/java/org/apache/sling/commons/metrics/rrd4j/impl/CodahaleMetricsReporter.java
index c0d96cb..fd874b0 100644
--- a/src/main/java/org/apache/sling/commons/metrics/rrd4j/impl/CodahaleMetricsReporter.java
+++ b/src/main/java/org/apache/sling/commons/metrics/rrd4j/impl/CodahaleMetricsReporter.java
@@ -49,6 +49,7 @@
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipOutputStream;
 
+import static org.apache.sling.commons.metrics.rrd4j.impl.RRD4JReporter.DEFAULT_PATH;
 import static org.apache.sling.commons.metrics.rrd4j.impl.RRD4JReporter.DEFAULT_STEP;
 
 @Component(
@@ -119,7 +120,7 @@
                         "available, otherwise relative to the current working " +
                         "directory."
         )
-        String path() default "metrics/metrics.rrd";
+        String path() default DEFAULT_PATH;
     }
 
     private MetricRegistry metricRegistry = new MetricRegistry();
@@ -128,31 +129,44 @@
     void activate(BundleContext context, Configuration config) throws Exception {
         LOG.info("Starting RRD4J Metrics reporter");
         configuration = config;
-        rrd = new File(config.path());
+        rrd = getSafePath(context, config);
+        reporter = RRD4JReporter.forRegistry(metricRegistry)
+                .withPath(rrd)
+                .withDatasources(config.datasources())
+                .withArchives(config.archives())
+                .withStep(config.step())
+                .build();
+        if (reporter != null) {
+            reporter.start(config.step(), TimeUnit.SECONDS);
+            LOG.info("Started RRD4J Metrics reporter: {}.", reporter);
+        } else {
+            LOG.warn("Illegal config will not start the RRD reporter. [path={}, datasources={}, archives={}, step={}].",
+                    rrd.getPath(), config.datasources(), config.archives(), config.step());
+        }
+    }
+
+    private static File getSafePath(BundleContext context, Configuration config) {
+        String path = config.path();
+        if (path == null || path.isEmpty()) {
+            path = DEFAULT_PATH;
+        }
+        File rrd = new File(path);
         if (!rrd.isAbsolute()) {
             String home = context.getProperty("sling.home");
             if (home != null) {
                 rrd = new File(home, rrd.getPath());
             }
         }
-
-        reporter = RRD4JReporter.forRegistry(metricRegistry)
-                .convertRatesTo(TimeUnit.SECONDS)
-                .convertDurationsTo(TimeUnit.MICROSECONDS)
-                .withPath(rrd)
-                .withDatasources(config.datasources())
-                .withArchives(config.archives())
-                .withStep(config.step())
-                .build();
-        reporter.start(config.step(), TimeUnit.SECONDS);
-        LOG.info("Started RRD4J Metrics reporter. Writing to " + rrd);
+        return rrd;
     }
 
     @Deactivate
     void deactivate() {
         LOG.info("Stopping RRD4J Metrics reporter");
-        reporter.stop();
-        reporter = null;
+        if (reporter != null) {
+            reporter.stop();
+            reporter = null;
+        }
         configuration = null;
         rrd = null;
         LOG.info("Stopped RRD4J Metrics reporter");
diff --git a/src/main/java/org/apache/sling/commons/metrics/rrd4j/impl/RRD4JReporter.java b/src/main/java/org/apache/sling/commons/metrics/rrd4j/impl/RRD4JReporter.java
index bc27a40..a0caef4 100644
--- a/src/main/java/org/apache/sling/commons/metrics/rrd4j/impl/RRD4JReporter.java
+++ b/src/main/java/org/apache/sling/commons/metrics/rrd4j/impl/RRD4JReporter.java
@@ -27,6 +27,7 @@
 import com.codahale.metrics.ScheduledReporter;
 import com.codahale.metrics.Timer;
 
+import org.rrd4j.core.Archive;
 import org.rrd4j.core.RrdDb;
 import org.rrd4j.core.RrdDef;
 import org.rrd4j.core.Sample;
@@ -39,11 +40,12 @@
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Properties;
+import java.util.Set;
 import java.util.SortedMap;
 import java.util.concurrent.TimeUnit;
 
@@ -56,6 +58,7 @@
 
     private static final String PROPERTIES_SUFFIX = ".properties";
     static final int DEFAULT_STEP = 5;
+    static final String DEFAULT_PATH = "metrics/metrics.rrd";
 
     private final Map<String, Integer> dictionary = new HashMap<>();
     private final RrdDb rrdDB;
@@ -66,8 +69,8 @@
 
     static class Builder {
         private MetricRegistry metricRegistry;
-        private TimeUnit ratesUnit;
-        private TimeUnit durationUnit;
+        private TimeUnit ratesUnit = TimeUnit.SECONDS;
+        private TimeUnit durationUnit = TimeUnit.MICROSECONDS;
         private File path = new File(".");
         private final List<String> indexedDS = new ArrayList<>();
         private final Map<String, Integer> dictionary = new HashMap<>();
@@ -79,35 +82,63 @@
         }
 
         Builder withPath(File path) {
+            if (path == null) {
+                LOGGER.warn("Illegal path value, will use default({}).", DEFAULT_PATH);
+                path = new File(DEFAULT_PATH);
+            }
             this.path = path;
             return this;
         }
 
         Builder withDatasources(String[] datasources) {
+            if (datasources == null) {
+                datasources = new String[0];
+            }
+
             this.indexedDS.clear();
             this.dictionary.clear();
 
             int i = 0;
             for (String ds : datasources) {
                 String[] tokens = ds.split(":");
-                if (tokens.length != 6) {
-                    throw new IllegalArgumentException("Invalid data source definition: " + ds);
+                if (tokens.length == 6) {
+                    String key = normalize(tokens[1]);
+                    tokens[1] = String.valueOf(i);
+                    try {
+                        indexedDS.add(checkDataSource(join(":", tokens)));
+                        dictionary.put(key, i);
+                    } catch (IllegalArgumentException ex) {
+                        LOGGER.warn("Ignoring malformed datasource {}.", ds);
+                    }
+                } else {
+                    LOGGER.warn("Ignoring malformed datasource {}.", ds);
                 }
-                dictionary.put(normalize(tokens[1]), i);
-                tokens[1] = String.valueOf(i);
-                this.indexedDS.add(checkDataSource(join(":", tokens)));
                 i++;
             }
             return this;
         }
 
         Builder withArchives(String[] archives) {
+            if (archives == null) {
+                archives = new String[0];
+            }
             this.archives.clear();
-            this.archives.addAll(Arrays.asList(archives));
+
+            for (String archive : archives) {
+                try {
+                    this.archives.add(checkArchive(archive));
+                } catch (IllegalArgumentException ex) {
+                    LOGGER.warn("Ignoring malformed archive {}.", archive);
+                }
+            }
             return this;
         }
 
         Builder withStep(int step) {
+            if (step <= 0) {
+                LOGGER.warn("Illegal step value, will use default({}).", DEFAULT_STEP);
+                step = DEFAULT_STEP;
+            }
             this.step = step;
             return this;
         }
@@ -122,20 +153,26 @@
             return this;
         }
 
-        ScheduledReporter build() throws IOException {
-            return new RRD4JReporter(metricRegistry, "RRD4JReporter",
-                    MetricFilter.ALL, ratesUnit, durationUnit, dictionary, createDef());
+        RRD4JReporter build() throws IOException {
+            if (indexedDS.isEmpty() || archives.isEmpty()) {
+                return null;
+            }
+            return new RRD4JReporter(metricRegistry, "RRD4JReporter", MetricFilter.ALL, ratesUnit, durationUnit,
+                    dictionary, createDef());
         }
 
-        private String checkDataSource(String ds)
-                throws IllegalArgumentException {
+        private String checkDataSource(String ds) throws IllegalArgumentException {
             new RrdDef("path").addDatasource(ds);
             return ds;
         }
 
+        private String checkArchive(String arch) throws IllegalArgumentException {
+            new RrdDef("path").addArchive(arch);
+            return arch;
+        }
+
         private RrdDef createDef() {
-            RrdDef def = new RrdDef(path.getPath());
-            def.setStep(step);
+            RrdDef def = new RrdDef(path.getPath(), step);
             for (String ds : indexedDS) {
                 def.addDatasource(ds);
             }
@@ -181,23 +218,23 @@
         try {
             Sample sample = rrdDB.createSample(System.currentTimeMillis() / 1000);
             for (Map.Entry<String, Gauge> entry : gauges.entrySet()) {
-                reported += update(sample, indexForName(entry.getKey()), entry.getValue());
+                reported += update(sample, entry.getKey(), entry.getValue());
             }
 
             for (Map.Entry<String, Counter> entry : counters.entrySet()) {
-                reported += update(sample, indexForName(entry.getKey()), entry.getValue());
+                reported += update(sample, entry.getKey(), entry.getValue());
             }
 
             for (Map.Entry<String, Histogram> entry : histograms.entrySet()) {
-                reported += update(sample, indexForName(entry.getKey()), entry.getValue());
+                reported += update(sample, entry.getKey(), entry.getValue());
             }
 
             for (Map.Entry<String, Meter> entry : meters.entrySet()) {
-                reported += update(sample, indexForName(entry.getKey()), entry.getValue());
+                reported += update(sample, entry.getKey(), entry.getValue());
             }
 
             for (Map.Entry<String, Timer> entry : timers.entrySet()) {
-                reported += update(sample, indexForName(entry.getKey()), entry.getValue());
+                reported += update(sample, entry.getKey(), entry.getValue());
             }
             sample.update();
         } catch (IOException e) {
@@ -218,48 +255,68 @@
         return name.replaceAll(":", "_");
     }
 
-    private int update(Sample sample, int nameIdx, Gauge g) {
+    private static void log(String key, String type, Number value) {
+        if (LOGGER.isDebugEnabled()) {
+            LOGGER.debug("Sample: {} ({}) = {}", key, type, value);
+        }
+    }
+
+    private int update(Sample sample, String key, Gauge g) {
+        int nameIdx = indexForName(key);
         if (nameIdx < 0) {
             return 0;
         }
         Object value = g.getValue();
         if (value instanceof Number) {
-            sample.setValue(nameIdx, ((Number) value).doubleValue());
+            double val = ((Number) value).doubleValue();
+            sample.setValue(nameIdx, val);
+            log(key, "gauge", val);
             return 1;
         }
         return 0;
     }
 
-    private int update(Sample sample, int nameIdx, Counter c) {
+    private int update(Sample sample, String key, Counter c) {
+        int nameIdx = indexForName(key);
         if (nameIdx < 0) {
             return 0;
         }
-        sample.setValue(nameIdx, c.getCount());
+        long val = c.getCount();
+        sample.setValue(nameIdx, val);
+        log(key, "counter", val);
         return 1;
     }
 
-    private int update(Sample sample, int nameIdx, Histogram h) {
+    private int update(Sample sample, String key, Histogram h) {
+        int nameIdx = indexForName(key);
         if (nameIdx < 0) {
             return 0;
         }
-        sample.setValue(nameIdx, h.getCount());
+        long val = h.getCount();
+        sample.setValue(nameIdx, val);
+        log(key, "histogram", val);
         return 1;
     }
 
-    private int update(Sample sample, int nameIdx, Timer t) {
+    private int update(Sample sample, String key, Timer t) {
+        int nameIdx = indexForName(key);
         if (nameIdx < 0) {
             return 0;
         }
-        sample.setValue(nameIdx, t.getCount());
+        long val = t.getCount();
+        sample.setValue(nameIdx, val);
+        log(key, "timer", val);
         return 1;
     }
 
-
-    private int update(Sample sample, int nameIdx, Meter m) {
+    private int update(Sample sample, String key, Meter m) {
+        int nameIdx = indexForName(key);
         if (nameIdx < 0) {
             return 0;
         }
-        sample.setValue(nameIdx, m.getCount());
+        long val = m.getCount();
+        sample.setValue(nameIdx, val);
+        log(key, "meter", val);
         return 1;
     }
 
@@ -277,7 +334,7 @@
         }
     }
 
-    private RrdDb createDB(RrdDef definition) throws IOException {
+    private static RrdDb createDB(RrdDef definition) throws IOException {
         File dbFile = new File(definition.getPath());
         if (!dbFile.getParentFile().exists()) {
             if (!dbFile.getParentFile().mkdirs()) {
@@ -302,7 +359,7 @@
         return db;
     }
 
-    private File renameDB(File dbFile) throws IOException {
+    private static File renameDB(File dbFile) throws IOException {
         // find a suitable suffix
         int idx = 0;
         while (new File(dbFile.getPath() + suffix(idx)).exists()) {
@@ -321,7 +378,7 @@
         return "." + idx;
     }
 
-    private void rename(Path path, String newName) throws IOException {
+    private static void rename(Path path, String newName) throws IOException {
         if (!Files.exists(path)) {
             // nothing to rename
             return;
@@ -329,4 +386,41 @@
         Path target = path.resolveSibling(newName);
         Files.move(path, target, REPLACE_EXISTING);
     }
+
+    long getStep() {
+        try {
+            return rrdDB.getHeader().getStep();
+        } catch (IOException e) {
+            LOGGER.error(e.getMessage(), e);
+        }
+        return -1;
+    }
+
+    String getPath() {
+        try {
+            return rrdDB.getCanonicalPath();
+        } catch (IOException e) {
+            LOGGER.error(e.getMessage(), e);
+        }
+        return "";
+    }
+
+    Set<String> getDatasources() {
+        return dictionary.keySet();
+    }
+
+    Set<String> getArchives() {
+        Set<String> archives = new HashSet<>();
+        for (int i = 0; i < rrdDB.getArcCount(); i++) {
+            Archive ar = rrdDB.getArchive(i);
+            archives.add(ar.toString());
+        }
+        return archives;
+    }
+
+    @Override
+    public String toString() {
+        return "RRD4JReporter [path=" + getPath() + ", datasources=" + getDatasources() + ", archives=" + getArchives()
+                + ", step=" + getStep() + ", dictionary=" + dictionary + "]";
+    }
 }
diff --git a/src/test/java/org/apache/sling/commons/metrics/rrd4j/impl/InvalidParamsTest.java b/src/test/java/org/apache/sling/commons/metrics/rrd4j/impl/InvalidParamsTest.java
new file mode 100644
index 0000000..3a69b43
--- /dev/null
+++ b/src/test/java/org/apache/sling/commons/metrics/rrd4j/impl/InvalidParamsTest.java
@@ -0,0 +1,152 @@
+/*
+ * 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.rrd4j.impl;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+
+import java.io.File;
+import java.util.Set;
+
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+import com.codahale.metrics.MetricRegistry;
+
+public class InvalidParamsTest {
+
+    @Rule
+    public TemporaryFolder folder = new TemporaryFolder(new File("target"));
+
+    private MetricRegistry registry = new MetricRegistry();
+
+    @Test
+    public void badStep() throws Exception {
+        File rrd = new File(folder.newFolder("InvalidParamsTest"), "metrics.rrd");
+        assertFalse(rrd.exists());
+
+        int step = -1;
+        String[] datasources = { "DS:oak_SESSION_LOGIN_COUNTER:COUNTER:300:0:U" };
+        String[] archives = { "RRA:AVERAGE:0.5:12:360" };
+
+        RRD4JReporter reporter = RRD4JReporter.forRegistry(registry).withPath(rrd).withDatasources(datasources)
+                .withArchives(archives).withStep(step).build();
+        assertNotNull(reporter);
+        assertTrue(rrd.exists());
+        assertTrue(reporter.getStep() == 5);
+
+        Set<String> ds = reporter.getDatasources();
+        assertEquals(1, ds.size());
+        assertEquals("oak_SESSION_LOGIN_COUNTER", ds.iterator().next());
+    }
+
+    @Test
+    public void noDatasource() throws Exception {
+        File rrd = new File(folder.newFolder("InvalidParamsTest"), "metrics.rrd");
+        assertFalse(rrd.exists());
+
+        String[] datasources = null;
+        String[] archives = { "RRA:AVERAGE:0.5:12:360" };
+
+        RRD4JReporter reporter = RRD4JReporter.forRegistry(registry).withPath(rrd).withDatasources(datasources)
+                .withArchives(archives).build();
+        assertNull(reporter);
+        assertFalse(rrd.exists());
+    }
+
+    @Test
+    public void badDatasource() throws Exception {
+        File rrd = new File(folder.newFolder("InvalidParamsTest"), "metrics.rrd");
+        assertFalse(rrd.exists());
+
+        String[] datasources = { "malformed:input" };
+        String[] archives = { "RRA:AVERAGE:0.5:12:360" };
+
+        RRD4JReporter reporter = RRD4JReporter.forRegistry(registry).withPath(rrd).withDatasources(datasources)
+                .withArchives(archives).build();
+        assertNull(reporter);
+        assertFalse(rrd.exists());
+    }
+
+    @Test
+    public void badDatasourceFilter() throws Exception {
+        File rrd = new File(folder.newFolder("InvalidParamsTest"), "metrics.rrd");
+        assertFalse(rrd.exists());
+
+        String[] datasources = { "DS:oak_SESSION_LOGIN_COUNTER:COUNTER:300:0:U", "malformed:input" };
+        String[] archives = { "RRA:AVERAGE:0.5:12:360" };
+
+        RRD4JReporter reporter = RRD4JReporter.forRegistry(registry).withPath(rrd).withDatasources(datasources)
+                .withArchives(archives).build();
+        assertNotNull(reporter);
+        assertTrue(rrd.exists());
+
+        Set<String> ds = reporter.getDatasources();
+        assertEquals(1, ds.size());
+        assertEquals("oak_SESSION_LOGIN_COUNTER", ds.iterator().next());
+    }
+
+    @Test
+    public void noArchive() throws Exception {
+        File rrd = new File(folder.newFolder("InvalidParamsTest"), "metrics.rrd");
+        assertFalse(rrd.exists());
+
+        String[] datasources = { "DS:oak_SESSION_LOGIN_COUNTER:COUNTER:300:0:U" };
+        String[] archives = null;
+
+        RRD4JReporter reporter = RRD4JReporter.forRegistry(registry).withPath(rrd).withDatasources(datasources)
+                .withArchives(archives).build();
+        assertNull(reporter);
+        assertFalse(rrd.exists());
+    }
+
+    @Test
+    public void badArchive() throws Exception {
+        File rrd = new File(folder.newFolder("InvalidParamsTest"), "metrics.rrd");
+        assertFalse(rrd.exists());
+
+        String[] datasources = { "DS:oak_SESSION_LOGIN_COUNTER:COUNTER:300:0:U" };
+        String[] archives = { "malformed:input" };
+
+        RRD4JReporter reporter = RRD4JReporter.forRegistry(registry).withPath(rrd).withDatasources(datasources)
+                .withArchives(archives).build();
+        assertNull(reporter);
+        assertFalse(rrd.exists());
+    }
+
+    @Test
+    public void badArchiveFilter() throws Exception {
+        File rrd = new File(folder.newFolder("InvalidParamsTest"), "metrics.rrd");
+        assertFalse(rrd.exists());
+
+        String[] datasources = { "DS:oak_SESSION_LOGIN_COUNTER:COUNTER:300:0:U" };
+        String[] archives = { "RRA:AVERAGE:0.5:12:360", "malformed:input" };
+
+        RRD4JReporter reporter = RRD4JReporter.forRegistry(registry).withPath(rrd).withDatasources(datasources)
+                .withArchives(archives).build();
+        assertNotNull(reporter);
+        assertTrue(rrd.exists());
+        Set<String> archs = reporter.getArchives();
+        assertEquals(1, archs.size());
+    }
+}