You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Daniel Estévez <no...@github.com> on 2017/06/13 15:26:23 UTC
[jclouds/jclouds-labs] Implements metrics and metricdefinitions API
(#396)
- https://docs.microsoft.com/en-us/rest/api/monitor/metrics
- https://docs.microsoft.com/en-us/rest/api/monitor/metricdefinitions
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/396
-- Commit Summary --
* Implements metrics and metricdefinitions API
-- File Changes --
M azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/AzureComputeApi.java (20)
M azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/AzureComputeProviderMetadata.java (4)
A azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/domain/Metric.java (64)
A azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/domain/MetricData.java (72)
A azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/domain/MetricDefinition.java (90)
A azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/features/MetricDefinitionsApi.java (52)
A azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/features/MetricsApi.java (51)
A azurecompute-arm/src/test/java/org/jclouds/azurecompute/arm/features/MetricDefinitionsApiLiveTest.java (130)
A azurecompute-arm/src/test/java/org/jclouds/azurecompute/arm/features/MetricDefinitionsApiMockTest.java (62)
A azurecompute-arm/src/test/java/org/jclouds/azurecompute/arm/features/MetricsApiLiveTest.java (142)
A azurecompute-arm/src/test/java/org/jclouds/azurecompute/arm/features/MetricsApiMockTest.java (71)
A azurecompute-arm/src/test/resources/metricdefinitions.json (25)
A azurecompute-arm/src/test/resources/metrics.json (19)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/396.patch
https://github.com/jclouds/jclouds-labs/pull/396.diff
--
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
Re: [jclouds/jclouds-labs] Implements metrics and metricdefinitions
API (#396)
Posted by Daniel Estévez <no...@github.com>.
@danielestevez pushed 1 commit.
d353bbc Returns numeric data as Double/Long
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/396/files/9312229b35c40debb33e66e324958102348dc751..d353bbc22a9c5b3b8e074c3506d480b47c5bba48
Re: [jclouds/jclouds-labs] Implements metrics and metricdefinitions
API (#396)
Posted by Ignasi Barrera <no...@github.com>.
Closed #396.
--
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#event-1141860292
Re: [jclouds/jclouds-labs] Implements metrics and metricdefinitions
API (#396)
Posted by Daniel Estévez <no...@github.com>.
@danielestevez pushed 1 commit.
9312229 Addresses comments by @nacx
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/396/files/44cd5b8fe62b6bae060b3995c438533cfc249dc2..9312229b35c40debb33e66e324958102348dc751
Re: [jclouds/jclouds-labs] Implements metrics and metricdefinitions
API (#396)
Posted by Daniel Estévez <no...@github.com>.
danielestevez commented on this pull request.
> + * 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);
That was my original idea but they're two different APIs in Azure and therefore deal with different entities and api-versions so i thought it'd be more maintainable if we'd keep it that way
--
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#discussion_r122482133
Re: [jclouds/jclouds-labs] Implements metrics and metricdefinitions
API (#396)
Posted by Ignasi Barrera <no...@github.com>.
Squashed and pushed to [master](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/47c4e72a) and [2.0.x](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/4183e2f9).
I also changed the metrics mock test so it uses the date service to parse dates (see https://github.com/jclouds/jclouds-labs/commit/dffc2bba7153a8222b18139298d76dcb2b4bbb6d). Otherwise, there are locale-dependent issues when parsing dates and the results were non-deterministic.
--
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#issuecomment-311582766
Re: [jclouds/jclouds-labs] Implements metrics and metricdefinitions
API (#396)
Posted by Daniel Estévez <no...@github.com>.
danielestevez commented on this pull request.
> +/**
+ *
+ */
+@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();
Ok, we could get Double for all fields but i still think we should return a String as Azure API does. If we force Double for let's say "total" this could be ok for metrics in bytes (as Network In) but would make no sense for metrics with no decimals (like Disk Read Operations/Sec that is a smaller count that could be even Int)
Since jclouds will not do any calculations with these values i think we should return them as they are and let the user decide how to parse them.
Thoughts?
--
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#discussion_r124293765
Re: [jclouds/jclouds-labs] Implements metrics and metricdefinitions
API (#396)
Posted by Ignasi Barrera <no...@github.com>.
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
Re: [jclouds/jclouds-labs] Implements metrics and metricdefinitions
API (#396)
Posted by Daniel Estévez <no...@github.com>.
danielestevez commented on this pull request.
> + * 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();
Correct. Will change to enum: None, Average, Count, Total, Minimum, Maximum seem to be the only possible values
--
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#discussion_r122482693
Re: [jclouds/jclouds-labs] Implements metrics and metricdefinitions
API (#396)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> +/**
+ *
+ */
+@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();
I still think a `Number` makes more sense. Having a number with `.0` decimal is not a big deal, as you can still do arithmetic operations, etc, normally. Letting the users how to parse them propagates unnecessary complexity to them.
--
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#discussion_r124302494
Re: [jclouds/jclouds-labs] Implements metrics and metricdefinitions
API (#396)
Posted by Daniel Estévez <no...@github.com>.
danielestevez commented on this pull request.
> +/**
+ *
+ */
+@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();
It's a String according to Azure documentation but yes, i think we could get them as Double/Long. I'll test this
--
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#discussion_r122482583