> On Jan. 7, 2017, 12:12 a.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/AmbariPerformanceMonitor.java, > > line 109 > > <https://reviews.apache.org/r/55278/diff/2/?file=1599025#file1599025line109> > > > > 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.
Jonathan, it is entirely asynchronous. Even pre-processing before sending to AMS is done asynchronously. > On Jan. 7, 2017, 12:12 a.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/AmbariPerformanceMonitor.java, > > lines 106-114 > > <https://reviews.apache.org/r/55278/diff/2/?file=1599025#file1599025line106> > > > > This kind of seems like a workflow problem, no? We shouldn't need to > > keep track of whether it was initialized. The AmbariPerformanceMonitor is designed to work with the DatabaseMetricsSource to send metrics to AMS. Since the AmbariPerformanceMonitor is instantiated through EclipseLink, and DatabaseMetricsSource is instantiated explicitly through MetricsService initialization, the Monitor might come up before the Source. Hence, I added this initialization check & retry. One can still use the inbuilt PeformanceMonitor to look at metrics through logs if not interested in visualizing them through AMS. > On Jan. 7, 2017, 12:12 a.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/metrics/system/impl/DatabaseMetricsSource.java, > > lines 81-85 > > <https://reviews.apache.org/r/55278/diff/2/?file=1599026#file1599026line81> > > > > 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. I will name this threadpool. IMO, 1 thread is enough since we have a maxmimum of 1 minute (monitor collection interval) to send metrics to AMS, which should be sufficient. - Aravindan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55278/#review160792 ----------------------------------------------------------- On Jan. 6, 2017, 10: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, 10: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 > >
