jclouds-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ignasi Barrera <notificati...@github.com>
Subject Re: [jclouds/jclouds-labs] Implements metrics and metricdefinitions API (#396)
Date Wed, 14 Jun 2017 10:12:21 GMT
nacx requested changes on this pull request.

Thanks @danielestevez!

> +    * The metrics API includes operations to get insights into entities within your
+    * subscription.
+    *
+    * @see <a href="https://docs.microsoft.com/en-us/rest/api/monitor/metrics">docs</a>
+    */
+   @Delegate
+   MetricsApi getMetricsApi(@PathParam("resourceid") String resourceid);
+
+   /**
+    * The metric definitions API includes operations to get insights available for entities
within your
+    * subscription.
+    *
+    * @see <a href="https://docs.microsoft.com/en-us/rest/api/monitor/metricdefinitions">docs</a>
+    */
+   @Delegate
+   MetricDefinitionsApi getMetricsDefinitionsApi(@PathParam("resourceid") String resourceid);

The two API classes have just one method. Worth having just one `Metrics` API that exposes
the metrics and the definitions?

> +
+   public abstract List<MetricData> data();
+
+   public abstract String id();
+
+   @Nullable
+   public abstract Metric.MetricName name();
+
+   public abstract String type();
+
+   public abstract String unit();
+
+   @SerializedNames({ "data", "id", "name", "type", "unit" })
+   public static Metric create(final List<MetricData> data, final String id, final
MetricName name, final String type,
+         final String unit) {
+      return new AutoValue_Metric(data, id, name, type, unit);

Enforce immutable and non-nullable lists. If this is a read-only object, not something users
will build and send to ARM in a request, then avoid having nullable collections (the typical
`tags` field of an object is a counter-example as we need to send it as `null`, so we enforce
immutability but not its presence):
```java
new AutoValue_Metric(data == null ? ImmutableList.of() : ImmutableList.copyOf(data), id, name,
type, unit);
```
Apply this pattern to all lists in the new model classes.

> +/**
+ *
+ */
+@AutoValue
+public abstract class MetricData {
+
+   /**
+    * The timestamp for the metric value in ISO 8601 format.
+    */
+   public abstract Date timeStamp();
+
+   /**
+    * The average value in the time range
+    */
+   @Nullable
+   public abstract String total();

Why is this a String? Can we better make this a Double/Long? The same applies to all other
numeric value fields.

> + * A Metric definition for a resource
+ */
+@AutoValue
+public abstract class MetricDefinition {
+
+   @Nullable
+   public abstract String resourceid();
+
+   public abstract MetricDefinition.MetricName name();
+
+   @Nullable
+   public abstract Boolean isDimensionRequired();
+
+   public abstract String unit();
+
+   public abstract String primaryAggregationType();

Is this a set of fixed values? Can we codify them in an enum?

> +
+import java.util.List;
+
+import org.jclouds.javax.annotation.Nullable;
+import org.jclouds.json.SerializedNames;
+
+import com.google.auto.value.AutoValue;
+
+/**
+ * A Metric definition for a resource
+ */
+@AutoValue
+public abstract class MetricDefinition {
+
+   @Nullable
+   public abstract String resourceid();

Better use camel case.

> +
+   public abstract String unit();
+
+   public abstract String primaryAggregationType();
+
+   public abstract List<MetricDefinition.MetricAvailability> metricAvailabilities();
+
+   public abstract String id();
+
+   @SerializedNames({ "resourceid", "name", "isDimensionRequired", "unit", "primaryAggregationType",
+         "metricAvailabilities", "id" })
+   public static MetricDefinition create(final String resourceid, final MetricName name,
+         final Boolean isDimensionRequired, final String unit, final String primaryAggregationType,
+         List<MetricAvailability> metricAvailabilities, final String id) {
+      return new AutoValue_MetricDefinition(resourceid, name, isDimensionRequired, unit,
primaryAggregationType,
+            metricAvailabilities, id);

Same comment about collection immutability.

> +      public abstract String timeGrain();
+
+      public abstract String retention();
+
+      MetricAvailability() {
+
+      }
+
+      @SerializedNames({ "timeGrain", "retention" })
+      public static MetricDefinition.MetricAvailability create(String timeGrain, String retention)
{
+         return new AutoValue_MetricDefinition_MetricAvailability(timeGrain, retention);
+      }
+   }
+
+   @AutoValue
+   public abstract static class MetricName {

This is already defined in the `Metric` class. Better extract it to a top level type.

> +   protected void initializeContext() {
+      super.initializeContext();
+      resourceDeleted = context.utils().injector().getInstance(Key.get(new TypeLiteral<Predicate<URI>>()
{
+      }, Names.named(TIMEOUT_RESOURCE_DELETED)));
+      api = view.unwrapApi(AzureComputeApi.class);
+   }
+
+   @Override
+   @BeforeClass
+   public void setupContext() {
+      super.setupContext();
+      NodeMetadata node = null;
+      try {
+         node = getOnlyElement(view.getComputeService().createNodesInGroup(group, 1, resourceGroup(group)));
+      } catch (RunNodesException e) {
+         e.printStackTrace();

Fail the test instead of silently printing the error to the stdout?

> +      api = view.unwrapApi(AzureComputeApi.class);
+   }
+
+   @Override
+   @BeforeClass
+   public void setupContext() {
+      super.setupContext();
+
+      dateFormat = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss'Z'");
+      startTime = dateFormat.format(new Date());
+
+      NodeMetadata node = null;
+      try {
+         node = getOnlyElement(view.getComputeService().createNodesInGroup(group, 1, resourceGroup(group)));
+      } catch (RunNodesException e) {
+         e.printStackTrace();

Same comment

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/396#pullrequestreview-43970275
Mime
View raw message