[Traffic Manager] Adding new preview api-version for Traffic Manager. This adds ONE new feature to Traffic Manager: HeatMap. by hrkulkarMsft · Pull Request #1580 · Azure/azure-rest-api-specs

hrkulkarMsft · 2017-08-23T17:34:40Z

This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.

PR information

The title of the PR is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
If applicable, the PR references the bug/issue that it fixes.
Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

I have read the contribution guidelines.
My spec meets the review criteria:
- The spec conforms to the Swagger 2.0 specification.
- The spec follows the guidelines described in the Swagger checklist document.
- Validation tools were run on swagger spec(s) and have all been fixed in this PR.

…ersion includes a new nested resource: Microsoft.Network/trafficManagerProfiles/heatMaps. This resource will be used to represent a heatmap of a Traffic Manager profile's traffic across a world map based off of the traffic's latency and volume. [Traffic Manager] Adding new feature to preview API: APP RUM. Also fixing some heatmap specifications.

msftclas · 2017-08-23T17:34:44Z

@hrkulkarMsft,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

anuchandy · 2017-08-23T17:39:48Z

@ravbhatnagar this is new new API version, please review and sign-off.

anuchandy · 2017-08-23T17:49:20Z

@hrkulkarMsft please update readme.md file to have a tag for this new api-version.

hrkulkarMsft · 2017-08-23T17:53:30Z

Should the readme.md still try to build non-preview api? Or should I add?

openapi-type: arm
tag: package-2017-09-preview

Tag: package-2017-09-preview

These settings apply only when --tag=package-2017-09-preview is specified on the command line.

input-file:
- Microsoft.Network/2017-09-01-preview/trafficmanager.json

anuchandy · 2017-08-23T18:56:11Z

@hrkulkarMsft, the readme file seems correct, it seems CI is failing because from the swagger you are referring HeatMap-GET.json but in the file system the file name is HeatMap-Get.json, please use the same casing in both places.

anuchandy · 2017-08-23T19:00:32Z

If you already have autorest installed I would recommend to run it locally against your swagger to catch linter errors, and fix them.

Assuming you are in the root folder of local clone of rest-api-spec, you can run this:

autorest --validation --azure-validator --message-format=json --input-file=./specification/trafficmanager/resource-manager/Microsoft.Network/2017-09-01-preview/trafficmanager.json

hrkulkarMsft · 2017-08-23T19:07:51Z

Thanks, my validator wasn't throwing that warning because git hadn't renamed the case remotely to match my local branch.

For Operations API -- our parent RP (Microsoft.Network) implements this should we still reference it in our Swagger?

anuchandy · 2017-08-23T19:21:00Z

For Operations API no need to reference to the parent network swagger. @ravbhatnagar please note that linter shows RPCViolation saying Operations API is missing but the same operation is implemented in the parent RP https://github.com/Azure/azure-rest-api-specs/tree/current/specification/network/resource-manager.

anuchandy · 2017-08-23T19:24:49Z

To repro semantic and model validation locally

install oav
npm install -g oav
run

oav validate-spec ./specification/trafficmanager/resource-manager/Microsoft.Network/2017-09-01-preview/trafficmanager.json
oav validate-example ./specification/trafficmanager/resource-manager/Microsoft.Network/2017-09-01-preview/trafficmanager.json

hrkulkarMsft · 2017-08-23T19:30:12Z

Thanks Anu, I was able to successfully run these validations this time.

One general question I have. I do get this warning:

{
"type": "Warning",
"code": "TrackedResourceListByImmediateParent",
"message": "The child tracked resource, 'heatMaps' with immediate parent 'Profile', must have a list by immediate parent operation.",
"id": "R3010",
"validationCategory": "RPCViolation",
"providerNamespace": "Microsoft.Network",
"resourceType": "trafficmanagerprofiles",
"sources": [
"file:///C:/wd/github/azure-rest-api-specs-pr/specification/trafficmanager/resource-manager/Microsoft.Network/2017-09-01-preview/trafficmanager.json:688:4 ($.paths["/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/
providers/Microsoft.Network/trafficmanagerprofiles/{profileName}/heatMaps/{heatMapType}"])"
]
}

For child resources, it seems it's generally the practice to have a sort of 'List' operations on them. However, in the case of this particular resource -- it could be very large. Would a list operation still be preferable to have, even if there is only one instance of this resource?

anuchandy · 2017-08-23T19:28:49Z

...ion/trafficmanager/resource-manager/Microsoft.Network/2017-09-01-preview/trafficmanager.json

+              "$ref": "#/definitions/CloudError"
+            }
+          }
+        },


Is this a long running operation? if yes mark it so using "x-ms-long-running-operation": true

No, it should be short-running.

thanks for confirming

anuchandy · 2017-08-23T19:29:11Z

...ion/trafficmanager/resource-manager/Microsoft.Network/2017-09-01-preview/trafficmanager.json

+              "$ref": "#/definitions/CloudError"
+            }
+          }
+        },


Is this a long running operation? if yes mark it so using "x-ms-long-running-operation": true

No, it should be short-running.

thanks for confirming

anuchandy · 2017-08-23T19:34:18Z

...ion/trafficmanager/resource-manager/Microsoft.Network/2017-09-01-preview/trafficmanager.json

+            "required": false,
+            "schema": {
+              "type": "string",
+              "enum": [""]


curious what exactly this enum is? if not used please remove.

Removed this.

anuchandy · 2017-08-23T19:45:13Z

@hrkulkarMsft regarding TrackedResourceListByImmediateParent RPCViolation - you mentioned that in the case of this particular resource -- it could be very large, if i understand correctly the number of HeapMap child resources associated with an instance of traffic manager profile resource can be huge. If that is the case, this could be a paged collection right? which can be expressed using x-ms-pageable extension, https://github.com/Azure/azure-rest-api-specs/blob/master/documentation/creating-swagger.md#Paging-x-ms-pageable.

hrkulkarMsft · 2017-08-23T19:48:21Z

@anuchandy Sorry, I should have been more specific. The payload can return close to 4MB in our preview version -- this will be right under ARM limit to keep it from needing pagination. If we decide to have multiple heatMap resources at some point then (i.e.: a history of a Profile's heatMap), I would imagine that a List operation could get large.

In current implementation we are specifically trying to limit the data to avoid pagination. If close to 4 MB should still require pagination, then I will add the extension.

anuchandy · 2017-08-23T19:59:36Z

@hrkulkarMsft thanks for clarifying, got it so there will be only one HeapMap child instance associated with a traffic manager profile instance, i.e. this is not a collection. I would let Gaurav comment on this. @ravbhatnagar we have RPCViolation warning in this case TrackedResourceListByImmediateParent, please share your thoughts.

ravbhatnagar

@hrkulkarMsft - please take a look at this feedback.

ravbhatnagar · 2017-08-23T21:20:40Z

...ion/trafficmanager/resource-manager/Microsoft.Network/2017-09-01-preview/trafficmanager.json

What is this key? Is it a secret? Shouldn't this be obtained via a POST?

The API signature is not correct for a GET or a PUT. it needs a segment for the resource name.

as discussed in person - make is default

ravbhatnagar · 2017-08-23T21:25:34Z

...ion/trafficmanager/resource-manager/Microsoft.Network/2017-09-01-preview/trafficmanager.json

How is this different from the "id" property in "Resource" model?

ravbhatnagar · 2017-08-23T21:27:31Z

...ion/trafficmanager/resource-manager/Microsoft.Network/2017-09-01-preview/trafficmanager.json

Is this a fixed finite set? Consider exposing this list through an API or make an enum. Or is this list same as the supported azure locations?

ravbhatnagar · 2017-08-23T21:29:09Z

...ion/trafficmanager/resource-manager/Microsoft.Network/2017-09-01-preview/trafficmanager.json

Are all properties in the model patchable?

ravbhatnagar · 2017-08-23T21:30:22Z

...ion/trafficmanager/resource-manager/Microsoft.Network/2017-09-01-preview/trafficmanager.json

camelcase resource type name -> trafficManagerProfiles

ravbhatnagar · 2017-08-23T21:32:19Z

...ion/trafficmanager/resource-manager/Microsoft.Network/2017-09-01-preview/trafficmanager.json

All properties are patchable?

ravbhatnagar · 2017-08-23T21:37:39Z

...ion/trafficmanager/resource-manager/Microsoft.Network/2017-09-01-preview/trafficmanager.json

Are you sure you want to return the entire Endpoint resource in the response of a Profile? You can just return the resource Ids for the endpoints.

You will have to do a linked access check to make sure user has access to these resources (endpoints).

ravbhatnagar · 2017-08-23T21:53:23Z

...ion/trafficmanager/resource-manager/Microsoft.Network/2017-09-01-preview/trafficmanager.json

This should be ISO8601 format

ravbhatnagar · 2017-08-23T21:53:42Z

...ion/trafficmanager/resource-manager/Microsoft.Network/2017-09-01-preview/trafficmanager.json

ISO 8601 format datetime?

ravbhatnagar · 2017-08-23T21:59:50Z

...ion/trafficmanager/resource-manager/Microsoft.Network/2017-09-01-preview/trafficmanager.json

Is this not the arm resource id of the endpoint

ravbhatnagar · 2017-08-23T23:13:04Z

...ion/trafficmanager/resource-manager/Microsoft.Network/2017-09-01-preview/trafficmanager.json

+      "description": "Class representing a Traffic Manager HeatMap properties."
+    },
+    "HeatMapEndpoint": {
+      "properties": {


lets keep just resourceid

hrkulkarMsft · 2017-08-24T20:58:25Z

@ravbhatnagar : Submitted a new iteration in the review with the changes we discussed.

• {heatMapsType}: can now only be “default” in value.
• Removed endpoint-identifying properties.
• Added query parameters.
• Removed App Rum to get HeatMap through for now, and will open separate review.

I’ll take note of the general API feedback to fix, but for this iteration would it be possible to only fix the HeatMap implementation, and then update the existing API comments later?

azuresdkciprbot · 2017-08-24T20:59:47Z

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/trafficmanager/resource-manager/readme.md
Before the PR: Warning(s): 2 Error(s): 1
After the PR: Warning(s): 3 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues

Send feedback and make AutoRest Linter Azure Bot smarter day by day!

Thanks for your co-operation.

Forcing git to rename HeatMap-Get.json to HeatMap-GET.json Fixing examples validation and json to match examples. Adding query parameters to HeatMap. Removing endpoint identifying properties. Must be accessed through additional GET. Removing RealUserMetrics API. Needs separate review. Adding query parameters for HeatMap.

azuresdkciprbot · 2017-08-24T21:25:33Z

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/trafficmanager/resource-manager/readme.md
Before the PR: Warning(s): 2 Error(s): 1
After the PR: Warning(s): 3 Error(s): 1

AutoRest Linter Guidelines | AutoRest Linter Issues

Send feedback and make AutoRest Linter Azure Bot smarter day by day!

Thanks for your co-operation.

ravbhatnagar · 2017-08-25T23:16:41Z

@hrkulkarMsft - sounds good.

anuchandy · 2017-08-30T17:46:41Z

RubyCoden is failing https://travis-ci.org/Azure/azure-rest-api-specs/jobs/268142263, checking with the codegen owners.

hrkulkarMsft · 2017-08-31T22:18:26Z

Hey, is there anything I need to do to unblock codegen?

anuchandy · 2017-08-31T23:41:20Z

hi @hrkulkarMsft sorry for the delay. No action required from your side now, we are tracking the code-gen issue here Azure/azure-sdk-for-ruby#944. Merging this PR.

Summary:

@ravbhatnagar as i mentioned above though there is a RPCViolation OperationsAPIImplementation reported by linter, the API is implemented in the parent Network swagger.
CI is failing in case of ruby code gen which ruby team is looking into. Mean time merging this PR with admin privilege.

hrkulkarMsft · 2017-08-31T23:43:13Z

Cool, thanks Anu! Changed the PR to better reflect these changes since the other feature was moved to a separate PR.

AutorestCI · 2017-08-31T23:49:33Z

No modification for AutorestCI/azure-sdk-for-node

AutorestCI · 2017-08-31T23:49:57Z

No modification for AutorestCI/azure-sdk-for-python

hrkulkarMsft added 2 commits August 23, 2017 10:10

Initial commit

9da7845

msftclas added the cla-not-required label Aug 23, 2017

anuchandy self-assigned this Aug 23, 2017

anuchandy added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Aug 23, 2017

anuchandy assigned ravbhatnagar Aug 23, 2017

hrkulkarMsft force-pushed the current branch from 6063592 to 2422086 Compare August 23, 2017 19:02

hrkulkarMsft force-pushed the current branch from 2422086 to c65aa5b Compare August 23, 2017 19:23

hrkulkarMsft force-pushed the current branch from c65aa5b to 12b380e Compare August 23, 2017 19:29

anuchandy reviewed Aug 23, 2017

View reviewed changes

ravbhatnagar suggested changes Aug 23, 2017

View reviewed changes

ravbhatnagar reviewed Aug 23, 2017

View reviewed changes

hrkulkarMsft force-pushed the current branch 5 times, most recently from c495de7 to 1ed2684 Compare August 24, 2017 20:52

hrkulkarMsft force-pushed the current branch from 1ed2684 to 78d04c5 Compare August 24, 2017 21:14

ravbhatnagar added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Aug 25, 2017

sarangan12 mentioned this pull request Aug 31, 2017

ClientModelExtensions condition - Sequence must have primary type of string Azure/azure-sdk-for-ruby#944

Closed

anuchandy merged commit a5e6b39 into Azure:current Aug 31, 2017

Conversation

hrkulkarMsft commented Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR information

Quality of Swagger

Uh oh!

msftclas commented Aug 23, 2017

Uh oh!

anuchandy commented Aug 23, 2017

Uh oh!

anuchandy commented Aug 23, 2017

Uh oh!

hrkulkarMsft commented Aug 23, 2017

Tag: package-2017-09-preview

Uh oh!

anuchandy commented Aug 23, 2017

Uh oh!

anuchandy commented Aug 23, 2017

Uh oh!

hrkulkarMsft commented Aug 23, 2017

Uh oh!

anuchandy commented Aug 23, 2017

Uh oh!

anuchandy commented Aug 23, 2017

Uh oh!

hrkulkarMsft commented Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hrkulkarMsft Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anuchandy commented Aug 23, 2017

Uh oh!

hrkulkarMsft commented Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anuchandy commented Aug 23, 2017

Uh oh!

ravbhatnagar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hrkulkarMsft commented Aug 23, 2017 •

edited

Loading

hrkulkarMsft commented Aug 23, 2017 •

edited

Loading

hrkulkarMsft Aug 23, 2017 •

edited

Loading

hrkulkarMsft commented Aug 23, 2017 •

edited

Loading

hrkulkarMsft commented Aug 24, 2017 •

edited

Loading

anuchandy commented Aug 31, 2017 •

edited

Loading