AMBARI-11041. API does not work for same service component metric with multiple function is requested. (swagle)

ambari-server/src/main/java/org/apache/ambari/server/controller/metrics/MetricsPropertyProvider.java

@@ -58,13 +58,13 @@ public abstract class MetricsPropertyProvider extends AbstractPropertyProvider {
     new MetricsPaddingMethod(MetricsPaddingMethod.PADDING_STRATEGY.ZEROS);
 
   protected MetricsPropertyProvider(Map<String, Map<String,
-    PropertyInfo>> componentPropertyInfoMap,
-                                 StreamProvider streamProvider,
-                                 ComponentSSLConfiguration configuration,
-                                 MetricHostProvider hostProvider,
-                                 String clusterNamePropertyId,
-                                 String hostNamePropertyId,
-                                 String componentNamePropertyId) {
+       PropertyInfo>> componentPropertyInfoMap,
+       StreamProvider streamProvider,
+       ComponentSSLConfiguration configuration,
+       MetricHostProvider hostProvider,
+       String clusterNamePropertyId,
+       String hostNamePropertyId,
+       String componentNamePropertyId) {
 
     super(componentPropertyInfoMap);
 

ambari-server/src/main/java/org/apache/ambari/server/controller/metrics/MetricsPropertyProviderProxy.java

@@ -83,15 +83,19 @@ public class MetricsPropertyProviderProxy extends AbstractPropertyProvider {
     }
   }
 
+  /**
+   * Allow delegates to support special properties not stack defined.
+   */
   @Override
   public Set<String> checkPropertyIds(Set<String> propertyIds) {
+    MetricsService metricsService = metricsServiceProvider.getMetricsServiceType();
     Set<String> checkedPropertyIds = super.checkPropertyIds(propertyIds);
-    for (String propertyId : checkedPropertyIds) {
-      if (propertyId.startsWith(ZERO_PADDING_PARAM)) {
-        checkedPropertyIds.remove(propertyId);
-      }
+
+    if (metricsService != null && metricsService.equals(TIMELINE_METRICS)) {
+      return amsPropertyProvider.checkPropertyIds(checkedPropertyIds);
+    } else {
+      return checkedPropertyIds;
     }
-    return checkedPropertyIds;
   }
 
   private void createHostPropertyProviders(Map<String, Map<String, PropertyInfo>> componentPropertyInfoMap,

ambari-server/src/main/java/org/apache/ambari/server/controller/metrics/timeline/AMSComponentPropertyProvider.java

@@ -27,6 +27,7 @@ import org.apache.ambari.server.controller.utilities.PredicateHelper;
 import org.apache.ambari.server.controller.utilities.StreamProvider;
 import org.apache.commons.lang.StringUtils;
 
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Map;
 import java.util.Set;
@@ -40,49 +41,57 @@ public class AMSComponentPropertyProvider extends AMSPropertyProvider {
                                           String clusterNamePropertyId,
                                           String componentNamePropertyId) {
 
-    super(componentPropertyInfoMap, streamProvider, configuration, hostProvider,
+    super(updateComponentMetricsWithAggregateFunctionSupport(componentPropertyInfoMap),
+      streamProvider, configuration, hostProvider,
       clusterNamePropertyId, null, componentNamePropertyId);
   }
 
-  @Override
-  protected Set<String> getRequestPropertyIds(Request request, Predicate predicate) {
-    Set<String> supportedPropertyIds = super.getRequestPropertyIds(request, predicate);
+  /**
+   * This method adds supported propertyInfo for component metrics with
+   * aggregate function ids. API calls with multiple aggregate functions
+   * applied to a single metric need this support.
+   *
+   * Currently this support is added only for Component metrics,
+   * this can be easily extended to all levels by moving this method to the
+   * base class: @AMSPropertyProvider.
+   */
+  private static Map<String, Map<String, PropertyInfo>> updateComponentMetricsWithAggregateFunctionSupport(
+        Map<String, Map<String, PropertyInfo>> componentMetrics) {
 
-    Set<String> unsupportedPropertyIds = new HashSet<String>(request.getPropertyIds());
-    if (predicate != null) {
-      unsupportedPropertyIds.addAll(PredicateHelper.getPropertyIds(predicate));
-    }
-    unsupportedPropertyIds.removeAll(supportedPropertyIds);
-    // Allow for aggregate function names to be a part of metrics names
-    if (!unsupportedPropertyIds.isEmpty()) {
-      for (String propertyId : unsupportedPropertyIds) {
-        String[] idWithFunctionArray = stripFunctionFromMetricName(propertyId);
-        if (idWithFunctionArray != null) {
-          propertyIdAggregateFunctionMap.put(idWithFunctionArray[0], idWithFunctionArray[1]);
-          supportedPropertyIds.add(idWithFunctionArray[0]);
-        }
-      }
+    if (componentMetrics == null || componentMetrics.isEmpty()) {
+      return componentMetrics;
     }
 
-    return supportedPropertyIds;
-  }
+    // For every component
+    for (Map<String, PropertyInfo> componentMetricInfo : componentMetrics.values()) {
+      Map<String, PropertyInfo> aggregateMetrics = new HashMap<String, PropertyInfo>();
+      // For every metric
+      for (Map.Entry<String, PropertyInfo> metricEntry : componentMetricInfo.entrySet()) {
+        // For each aggregate function id
+        for (String identifierToAdd : aggregateFunctionIdentifierMap.values()) {
+          String metricInfoKey = metricEntry.getKey() + identifierToAdd;
+          // This disallows metric key suffix of the form "._sum._sum" for
+          // the sake of avoiding duplicates
+          if (componentMetricInfo.containsKey(metricInfoKey)) {
+            continue;
+          }
 
-  /**
-   * Return array of function identifier and metricsName stripped of function
-   * identifier for metricsNames with aggregate function identifiers as
-   * trailing suffixes.
-   */
-  protected String[] stripFunctionFromMetricName(String propertyId) {
-    for (Map.Entry<AGGREGATE_FUNCTION_IDENTIFIER, String> identifierEntry :
-        aggregateFunctionIdentifierMap.entrySet()) {
-      if (propertyId.endsWith(identifierEntry.getValue())) {
-        String[] retVal = new String[2];
-        retVal[0] = StringUtils.removeEnd(propertyId, identifierEntry.getValue());
-        retVal[1] = identifierEntry.getValue();
-        return retVal;
+          PropertyInfo propertyInfo = metricEntry.getValue();
+          PropertyInfo metricInfoValue = new PropertyInfo(
+            propertyInfo.getPropertyId() + identifierToAdd,
+            propertyInfo.isTemporal(),
+            propertyInfo.isPointInTime());
+          metricInfoValue.setAmsHostMetric(propertyInfo.isAmsHostMetric());
+          metricInfoValue.setAmsId(propertyInfo.getAmsId());
+          metricInfoValue.setUnit(propertyInfo.getUnit());
+
+          aggregateMetrics.put(metricInfoKey, metricInfoValue);
+        }
       }
+      componentMetricInfo.putAll(aggregateMetrics);
     }
-    return null;
+
+    return componentMetrics;
   }
 
   @Override

ambari-server/src/main/java/org/apache/ambari/server/controller/metrics/timeline/AMSPropertyProvider.java

@@ -53,6 +53,7 @@ import java.util.regex.Pattern;
 import static org.apache.ambari.server.Role.HBASE_MASTER;
 import static org.apache.ambari.server.Role.HBASE_REGIONSERVER;
 import static org.apache.ambari.server.Role.METRICS_COLLECTOR;
+import static org.apache.ambari.server.controller.metrics.MetricsPaddingMethod.ZERO_PADDING_PARAM;
 import static org.apache.ambari.server.controller.metrics.MetricsServiceProvider.MetricsService.TIMELINE_METRICS;
 import static org.codehaus.jackson.map.annotate.JsonSerialize.Inclusion;
 
@@ -70,8 +71,6 @@ public abstract class AMSPropertyProvider extends MetricsPropertyProvider {
   }
   protected static final EnumMap<AGGREGATE_FUNCTION_IDENTIFIER, String> aggregateFunctionIdentifierMap =
     new EnumMap<AGGREGATE_FUNCTION_IDENTIFIER, String>(AGGREGATE_FUNCTION_IDENTIFIER.class);
-  // Map to store aggregate functions that apply to metrics.
-  protected Map<String, String> propertyIdAggregateFunctionMap = new HashMap<String, String>();
 
   static {
     TIMELINE_APPID_MAP.put(HBASE_MASTER.name(), "HBASE");
@@ -113,6 +112,32 @@ public abstract class AMSPropertyProvider extends MetricsPropertyProvider {
     return componentName;
   }
 
+  /**
+   * Support properties with aggregate functions and metrics padding method.
+   */
+  @Override
+  public Set<String> checkPropertyIds(Set<String> propertyIds) {
+    for (String propertyId : propertyIds) {
+      if (propertyId.startsWith(ZERO_PADDING_PARAM)
+          || hasAggregateFunctionSuffix(propertyId)) {
+        propertyIds.remove(propertyId);
+      }
+    }
+    return propertyIds;
+  }
+
+  /**
+   * Check if property ends with a trailing suffix
+   */
+  protected boolean hasAggregateFunctionSuffix(String propertyId) {
+    for (String aggregateFunctionId : aggregateFunctionIdentifierMap.values()) {
+      if (propertyId.endsWith(aggregateFunctionId)) {
+        return true;
+      }
+    }
+    return false;
+  }
+
   /**
    * The information required to make a single call to the Metrics service.
    */
@@ -437,9 +462,6 @@ public abstract class AMSPropertyProvider extends MetricsPropertyProvider {
       }
     }
 
-    // Clear function map
-    propertyIdAggregateFunctionMap.clear();
-
     return resources;
   }
 
@@ -515,10 +537,7 @@ public abstract class AMSPropertyProvider extends MetricsPropertyProvider {
         for (Map.Entry<String, PropertyInfo> entry : propertyInfoMap.entrySet()) {
           String propertyId = entry.getKey();
           PropertyInfo propertyInfo = entry.getValue();
-          // For regex properties this propertyId != id
-          String amsPropertyId = getPropertyIdWithAggregateFunctionId(propertyInfo, id, false);
-          String ambariPropertyId = getPropertyIdWithAggregateFunctionId(propertyInfo, id, true);
-          TemporalInfo temporalInfo = request.getTemporalInfo(ambariPropertyId);
+          TemporalInfo temporalInfo = request.getTemporalInfo(id);
 
           if ((temporalInfo == null && propertyInfo.isPointInTime()) ||
             (temporalInfo != null && propertyInfo.isTemporal())) {
@@ -531,10 +550,10 @@ public abstract class AMSPropertyProvider extends MetricsPropertyProvider {
               requests.put(temporalInfo, metricsRequest);
             }
             metricsRequest.putResource(getHostName(resource), resource);
-            metricsRequest.putPropertyId(amsPropertyId, propertyId);
+            metricsRequest.putPropertyId(propertyInfo.getPropertyId(), propertyId);
             // If request is for a host metric we need to create multiple requests
             if (propertyInfo.isAmsHostMetric()) {
-              metricsRequest.putHosComponentHostMetric(amsPropertyId);
+              metricsRequest.putHosComponentHostMetric(propertyInfo.getPropertyId());
             }
           }
         }
@@ -544,22 +563,6 @@ public abstract class AMSPropertyProvider extends MetricsPropertyProvider {
     return requestMap;
   }
 
-  /**
-   * Return a property Id with aggregate function identifier.
-   * @param propertyInfo @PropertyInfo
-   * @param propertyId Property Id from request / predicate
-   * @param ambariPropertyId True: Return ambari property id,
-   *                         else return id using PropertyInfo
-   */
-  private String getPropertyIdWithAggregateFunctionId(PropertyInfo propertyInfo,
-                                  String propertyId, boolean ambariPropertyId) {
-    if (propertyIdAggregateFunctionMap.containsKey(propertyId)) {
-      return (ambariPropertyId ? propertyId : propertyInfo.getPropertyId()) +
-        propertyIdAggregateFunctionMap.get(propertyId);
-    }
-    return ambariPropertyId ? propertyId : propertyInfo.getPropertyId();
-  }
-
   static URIBuilder getAMSUriBuilder(String hostname, int port) {
     URIBuilder uriBuilder = new URIBuilder();
     uriBuilder.setScheme("http");

ambari-server/src/test/java/org/apache/ambari/server/controller/internal/StackDefinedPropertyProviderTest.java

@@ -29,6 +29,7 @@ import java.util.Set;
 
 import org.apache.ambari.server.controller.jmx.TestStreamProvider;
 import org.apache.ambari.server.controller.metrics.JMXPropertyProviderTest;
+import org.apache.ambari.server.controller.metrics.MetricsServiceProvider;
 import org.apache.ambari.server.controller.metrics.ganglia.GangliaPropertyProviderTest.TestGangliaHostProvider;
 import org.apache.ambari.server.controller.metrics.ganglia.GangliaPropertyProviderTest.TestGangliaServiceProvider;
 import org.apache.ambari.server.controller.spi.Predicate;
@@ -1034,4 +1035,54 @@ public class StackDefinedPropertyProviderTest {
 
   }
 
+  @Test
+  public void testPopulateResourcesWithAggregateFunctionMetrics() throws Exception {
+
+    String metric = "metrics/rpc/NumOpenConnections._sum";
+
+    org.apache.ambari.server.controller.metrics.ganglia.TestStreamProvider streamProvider =
+      new org.apache.ambari.server.controller.metrics.ganglia.TestStreamProvider("ams/aggregate_component_metric.json");
+
+    JMXPropertyProviderTest.TestJMXHostProvider jmxHostProvider = new JMXPropertyProviderTest.TestJMXHostProvider(true);
+    TestGangliaHostProvider hostProvider = new TestGangliaHostProvider();
+    MetricsServiceProvider serviceProvider = new MetricsServiceProvider() {
+      @Override
+      public MetricsService getMetricsServiceType() {
+        return MetricsService.TIMELINE_METRICS;
+      }
+    };
+
+    StackDefinedPropertyProvider propertyProvider = new StackDefinedPropertyProvider(
+      Resource.Type.Component,
+      jmxHostProvider,
+      hostProvider,
+      serviceProvider,
+      streamProvider,
+      PropertyHelper.getPropertyId("HostRoles", "cluster_name"),
+      PropertyHelper.getPropertyId("HostRoles", "host_name"),
+      PropertyHelper.getPropertyId("HostRoles", "component_name"),
+      PropertyHelper.getPropertyId("HostRoles", "state"),
+      new EmptyPropertyProvider(),
+      new EmptyPropertyProvider());
+
+
+    Resource resource = new ResourceImpl(Resource.Type.Component);
+
+    resource.setProperty("HostRoles/cluster_name", "c1");
+    resource.setProperty("HostRoles/service_name", "HBASE");
+    resource.setProperty(HOST_COMPONENT_COMPONENT_NAME_PROPERTY_ID, "HBASE_REGIONSERVER");
+
+    // only ask for one property
+    Map<String, TemporalInfo> temporalInfoMap = new HashMap<String, TemporalInfo>();
+    temporalInfoMap.put(metric, new TemporalInfoImpl(1429824611300L, 1429825241400L, 1L));
+    Request request = PropertyHelper.getReadRequest(Collections.singleton(metric), temporalInfoMap);
+
+    Assert.assertEquals(1, propertyProvider.populateResources(Collections.singleton(resource), request, null).size());
+
+    Assert.assertEquals(4, PropertyHelper.getProperties(resource).size());
+    Assert.assertNotNull(resource.getPropertyValue(metric));
+    Number[][] metricsArray = (Number[][]) resource.getPropertyValue(metric);
+    Assert.assertEquals(32, metricsArray.length);
+  }
+
 }

ambari-server/src/test/java/org/apache/ambari/server/controller/metrics/ganglia/GangliaPropertyProviderTest.java

@@ -789,11 +789,7 @@ public class GangliaPropertyProviderTest {
     Assert.assertEquals(11, PropertyHelper.getProperties(resource).size());
     Assert.assertNotNull(resource.getPropertyValue(FLUME_CHANNEL_CAPACITY_PROPERTY));
   }
-  
-
 
-
-  
   private boolean isUrlParamsEquals(URIBuilder actualUri, URIBuilder expectedUri) {
     for (final NameValuePair expectedParam : expectedUri.getQueryParams()) {
       NameValuePair actualParam = (NameValuePair) CollectionUtils.find(actualUri.getQueryParams(), new Predicate() {

ambari-server/src/test/java/org/apache/ambari/server/controller/metrics/timeline/AMSPropertyProviderTest.java

@@ -404,7 +404,7 @@ public class AMSPropertyProviderTest {
     uriBuilder.addParameter("startTime", "1429824611300");
     uriBuilder.addParameter("endTime", "1429825241400");
     Assert.assertEquals(uriBuilder.toString(), streamProvider.getLastSpec());
-    Number[][] val = (Number[][]) res.getPropertyValue(propertyProvider.stripFunctionFromMetricName(propertyId)[0]);
+    Number[][] val = (Number[][]) res.getPropertyValue(propertyId);
     Assert.assertEquals(32, val.length);
   }
 

ambari-server/src/test/resources/stacks/HDP/2.0.5/services/HBASE/metrics.json

@@ -3186,5 +3186,21 @@
         }
       }
     ]
+  },
+  "HBASE_REGIONSERVER": {
+    "Component": [
+      {
+        "type": "ganglia",
+        "metrics": {
+          "default": {
+            "metrics/rpc/NumOpenConnections": {
+              "metric": "rpc.rpc.NumOpenConnections",
+              "pointInTime": true,
+              "temporal": true
+            }
+          }
+        }
+      }
+    ]
   }
 }