----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55278/#review160792 -----------------------------------------------------------
ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/AmbariPerformanceMonitor.java (lines 74 - 75) <https://reviews.apache.org/r/55278/#comment232006> You're not calling super here. We're making them choose between AmbariPerformanceMonitor and the regular PerformanceMonitor? That's probably OK - just wanted to point out and make sure the behavior is desired. ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/AmbariPerformanceMonitor.java (lines 106 - 114) <https://reviews.apache.org/r/55278/#comment232004> This kind of seems like a workflow problem, no? We shouldn't need to keep track of whether it was initialized. ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/AmbariPerformanceMonitor.java (line 109) <https://reviews.apache.org/r/55278/#comment232007> How do the internal workings of metricsSource.publish(...) work? If it's synchronous in any way, this could cause a massive performance issue. If it simply queues data to be pushed to Ambari Metrics and returns immediately, then I think we're OK and you can drop the issue. I'm just wanting to make sure we're not sneaking in a major bottleneck. ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/AmbariPerformanceMonitor.java (line 117) <https://reviews.apache.org/r/55278/#comment232005> Doc. ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/DatabaseMetricsSource.java (line 65) <https://reviews.apache.org/r/55278/#comment232010> Doc. ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/DatabaseMetricsSource.java (lines 81 - 85) <https://reviews.apache.org/r/55278/#comment232009> Should this be configurable? Is 1 thread always enough? Anyway - can we also name this threadpool so we know what it is when looking at dumps. ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/DatabaseMetricsSource.java (line 87) <https://reviews.apache.org/r/55278/#comment232008> Doc. ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/DatabaseMetricsSource.java (line 89) <https://reviews.apache.org/r/55278/#comment232012> For readability, can we make this a named inner class which implements Runnable? ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/DatabaseMetricsSource.java (lines 175 - 177) <https://reviews.apache.org/r/55278/#comment232011> Doc. - Jonathan Hurley On Jan. 6, 2017, 5:44 p.m., Aravindan Vijayan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55278/ > ----------------------------------------------------------- > > (Updated Jan. 6, 2017, 5:44 p.m.) > > > Review request for Ambari, Alejandro Fernandez, Dmytro Sen, Jonathan Hurley, > Sumit Mohanty, and Sid Wagle. > > > Bugs: AMBARI-17596 > https://issues.apache.org/jira/browse/AMBARI-17596 > > > Repository: ambari > > > Description > ------- > > Extend the eclipse link PeformanceMonitor utility to collect and send metrics > to AMS. > Counter, Timer and Avg time (Timer/Counter) metrics are tracked. > > The metrics.properties file has enough information to know how to configure > the Database metrics source to collect metrics of interest. > > > Diffs > ----- > > ambari-server/conf/unix/metrics.properties 3ee22d6 > ambari-server/conf/windows/metrics.properties 3ee22d6 > > ambari-server/src/main/java/org/apache/ambari/server/metrics/system/MetricsService.java > 4a613f0 > > ambari-server/src/main/java/org/apache/ambari/server/metrics/system/MetricsSource.java > 400dcb6 > > ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/AbstractMetricsSource.java > 6bdd0ba > > ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/AmbariMetricSinkImpl.java > be24988 > > ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/AmbariPerformanceMonitor.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/DatabaseMetricsSource.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/JvmMetricsSource.java > cb9f275 > > ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/MetricsConfiguration.java > ca83a53 > > ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/MetricsServiceImpl.java > d0d2e69 > > ambari-server/src/test/java/org/apache/ambari/server/metric/system/impl/JvmMetricsSourceTest.java > 9f649b4 > > ambari-server/src/test/java/org/apache/ambari/server/metric/system/impl/MetricsServiceTest.java > 4029f25 > > ambari-server/src/test/java/org/apache/ambari/server/metric/system/impl/MetricsSourceTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/metric/system/impl/TestAmbariMetricsSinkImpl.java > 3565504 > > ambari-server/src/test/java/org/apache/ambari/server/metric/system/impl/TestMetricsSource.java > acf1586 > ambari-server/src/test/resources/metrics.properties 5eee064 > > Diff: https://reviews.apache.org/r/55278/diff/ > > > Testing > ------- > > Manually tested. > Unit tests added. > > > File Attachments > ---------------- > > Sample Grafana dashboard for AmbariServer DB metrics > > https://reviews.apache.org/media/uploaded/files/2017/01/06/a6f88cb6-743f-44ed-a851-3e959072b176__Screen_Shot_2017-01-06_at_12.59.00_PM.png > > > Thanks, > > Aravindan Vijayan > >
