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
