Implements metrics and metricdefinitions API #396
Conversation
danielestevez
commented
Jun 13, 2017
danielestevez
commented
- https://docs.microsoft.com/en-us/rest/api/monitor/metrics
- https://docs.microsoft.com/en-us/rest/api/monitor/metricdefinitions
| * @see <a href="https://docs.microsoft.com/en-us/rest/api/monitor/metricdefinitions">docs</a> | ||
| */ | ||
| @Delegate | ||
| MetricDefinitionsApi getMetricsDefinitionsApi(@PathParam("resourceid") String resourceid); |
There was a problem hiding this comment.
The two API classes have just one method. Worth having just one Metrics API that exposes the metrics and the definitions?
There was a problem hiding this comment.
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
| @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); |
There was a problem hiding this comment.
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):
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.
| * The average value in the time range | ||
| */ | ||
| @Nullable | ||
| public abstract String total(); |
There was a problem hiding this comment.
Why is this a String? Can we better make this a Double/Long? The same applies to all other numeric value fields.
There was a problem hiding this comment.
It's a String according to Azure documentation but yes, i think we could get them as Double/Long. I'll test this
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
|
||
| public abstract String unit(); | ||
|
|
||
| public abstract String primaryAggregationType(); |
There was a problem hiding this comment.
Is this a set of fixed values? Can we codify them in an enum?
There was a problem hiding this comment.
Correct. Will change to enum: None, Average, Count, Total, Minimum, Maximum seem to be the only possible values
| public abstract class MetricDefinition { | ||
|
|
||
| @Nullable | ||
| public abstract String resourceid(); |
| 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); |
There was a problem hiding this comment.
Same comment about collection immutability.
| } | ||
|
|
||
| @AutoValue | ||
| public abstract static class MetricName { |
There was a problem hiding this comment.
This is already defined in the Metric class. Better extract it to a top level type.
| try { | ||
| node = getOnlyElement(view.getComputeService().createNodesInGroup(group, 1, resourceGroup(group))); | ||
| } catch (RunNodesException e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
Fail the test instead of silently printing the error to the stdout?
| try { | ||
| node = getOnlyElement(view.getComputeService().createNodesInGroup(group, 1, resourceGroup(group))); | ||
| } catch (RunNodesException e) { | ||
| e.printStackTrace(); |
