Consumption specifications for cost insights and reporting operations by shalinved · Pull Request #3171 · Azure/azure-rest-api-specs

shalinved · 2018-06-01T04:00:52Z

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.

msftclas · 2018-06-01T04:01:03Z

All CLA requirements met.

AutorestCI · 2018-06-01T04:01:30Z

Automation for azure-libraries-for-java

Nothing to generate for azure-libraries-for-java

AutorestCI · 2018-06-01T04:01:38Z

Automation for azure-sdk-for-python

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-python#2158

AutorestCI · 2018-06-01T04:02:16Z

Automation for azure-sdk-for-node

A PR has been created for you based on this PR content.

Once this PR will be merged, content will be added to your service PR:
Azure/azure-sdk-for-node#2700

AutorestCI · 2018-06-01T04:05:22Z

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

AutorestCI · 2018-06-01T04:06:12Z

Automation for azure-sdk-for-go

A PR has been created for you based on this PR content.

Once this PR will be merged, content will be added to your service PR:
Azure/azure-sdk-for-go#2076

lmazuel · 2018-06-01T17:13:05Z

Hi @shalinved
Since it's a new API, adding @ravbhatnagar for ARM feedback
Please also fix all CI issues. For instance:

ERROR: Referenced file 'file:///tmp/tmp6a6_3m9v/rest/specification/consumption/resource-manager/Microsoft.Consumption/stable/2018-05-31/e./examples/ReportConfigDelete.json' not found

Thanks!

shalinved · 2018-06-01T17:26:28Z

@lmazuel - I just pushed a commit to fix bad reference for ReportConfigDelete.
@ravbhatnagar - Hi, Gaurav, these APIs are the ones we talked about some time back and had a casual discussion about. Let me know if you would like to schedule a formal review meeting.

lmazuel · 2018-06-04T23:34:42Z

@fearthecowboy Autorest fails saying this:

FATAL: System.InvalidOperationException: Method with x-ms-odata needs to have "$filter" parameter.

But all linter rules are fine and green. Is this a linter problem? Autorest problem?

fearthecowboy · 2018-06-05T12:43:39Z

I would think there needs to be a linter rule for this; https://github.com/Azure/autorest.common/blob/e85fe8a6a6c89a62c93708f7da4a8c555590c436/src/AzureExtensions.cs shows that it expects the $filter ...

ravbhatnagar

@shalinved - lets schedule a quick sync to go over these. I have a few questions about these. Also, please loop in folks from azureapirbcore - @johanste @devigned @fearthecowboy

ravbhatnagar · 2018-06-06T01:11:36Z

...cation/consumption/resource-manager/Microsoft.Consumption/stable/2018-05-31/consumption.json

+        }
+      }
+    },
+    "/subscriptions/{subscriptionId}/providers/Microsoft.Consumption/reportconfigs/{reportConfigName}": {


Is reportConfigs created under a resource group or under a subscription?

Shouldnt it be created under an RG

We allow report configurations to be created under a subscription as well as under a RG. This determines scope of the data in the report. This is similar to what we do currently in Consumption Budgets as well.

ravbhatnagar · 2018-06-06T01:13:01Z

...cation/consumption/resource-manager/Microsoft.Consumption/stable/2018-05-31/consumption.json

+          "type": "string",
+          "description": "Resource type."
+        },
+        "tags": {


why this is readonly?

ravbhatnagar · 2018-06-06T22:33:02Z

Had a call with Shalin - 3 things -

POST APIs for insights - change the name to represent an action - ex- generateInsight. We discussed about making these PUT and enabling GET. Currently someone needs this action permission to call this API. Reader cant get the insight. RP is fine with this behavior
Reference to storage account should be through resourceID with Linked access check enabled. - blocking feedback
Make sure the size of data coming back in response for dimensions is under the ARM limits. As it can grow, it can cause problems.

ms-premp · 2018-06-07T07:57:13Z

adding @ms-premp

ravbhatnagar · 2018-06-12T19:05:28Z

@shalinved - any updates on this? @ms-premp ?

shalinved · 2018-06-12T22:05:31Z

@ravbhatnagar - Resource id changes are done. I am waiting for a decision on the name of Insights API.

shalinved · 2018-06-12T22:24:20Z

The names we came up with for the API are - QueryUsage and/or AnalyzeUsage. Do you have any preference?

shalinved · 2018-06-13T18:19:55Z

@marbing - Adding Matt to the discussion

shalinved · 2018-06-13T23:28:11Z

@ravbhatnagar - I have pushed changes that would take care of points 1 and 2 above.
I have modified our request contract to accept an ARM resource id for the storage account where reports will be delivered. We will use ARM linked access check to validate user has read permissions on the resource while creating a report configuration.
I have also updated the Insights API to AnalyzeUsage.
We do have checks in place to make sure we don't go above ARM limits for Dimensions API. The API is pageable with a page size of 1000 records.
Could you please review the changes.

ravbhatnagar · 2018-06-14T08:54:48Z

looks good

lmazuel · 2018-06-14T18:29:23Z

...cation/consumption/resource-manager/Microsoft.Consumption/stable/2018-05-31/consumption.json

+          "format": "date-time",
+          "type": "string"
+        },
+        "nextLink": {


Why would this object get a nextLink? Just checking that it's accurate, since it's not usual and SDK won't be able to do anything with that, and polling would be manual.

Hi @imazuel
This is intentional. The response here is a single object and not a list of object. The single object however, can have a very big payload in data field. Hence, the need for pagination. We are aware that SDKs will not be able to parse it automatically and we will have additional work to support this in SDK.
This pattern is already used in one other API v.i.z. Pricesheets. We had multiple back and forth discussions with ARM team and this is the only way we can come up with since the response here is a single ARM resource object and not an array of ARM resources.

lmazuel

One question to confirm something unusual, otherwise LGTM

jhendrixMSFT · 2018-06-19T22:14:21Z

@AutorestCI regenerate azure-sdk-for-go

Consumption specifications for cost insights and reporting operations

e2fbb50

azuresdkci assigned lmazuel Jun 1, 2018

azuresdkci requested a review from lmazuel June 1, 2018 04:02

lmazuel added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Jun 1, 2018

Fixed $ref for ReportConfigDelete

dcb8e98

AutorestCI mentioned this pull request Jun 1, 2018

[AutoPR consumption/resource-manager] Consumption specifications for cost insights and reporting operations Azure/azure-sdk-for-go#1945

Closed

Silly me!! Fixed bad reference again

4e8e15b

AutorestCI mentioned this pull request Jun 1, 2018

[AutoPR consumption/resource-manager] Consumption specifications for cost insights and reporting operations Azure/azure-sdk-for-go#1946

Closed

Shalin Ved added 4 commits June 1, 2018 13:39

Fixed schema validation issues

e702517

Fixing validation errors around datetime and dynamic respones

4aaf2ca

Fixed parameters in examples

b51ba78

Fixed parameter name in example

5b1ce95

Removed x-ms-odata method for POST op

1d58423

AutorestCI mentioned this pull request Jun 5, 2018

[AutoPR consumption/resource-manager] Consumption specifications for cost insights and reporting operations Azure/azure-sdk-for-python#2692

Merged

ravbhatnagar reviewed Jun 6, 2018

View reviewed changes

AutorestCI mentioned this pull request Jun 6, 2018

[AutoPR consumption/resource-manager] Consumption specifications for cost insights and reporting operations Azure/azure-sdk-for-node#2949

Closed

Shalin Ved added 2 commits June 13, 2018 11:48

Fixed ARM review comments

03c3377

Merging upstream to fork

7eea456

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 Jun 14, 2018

lmazuel reviewed Jun 14, 2018

View reviewed changes

lmazuel approved these changes Jun 14, 2018

View reviewed changes

lmazuel merged commit 30f1846 into Azure:master Jun 14, 2018

AutorestCI mentioned this pull request Jun 19, 2018

[AutoPR consumption/resource-manager] Consumption specifications for cost insights and reporting operations Azure/azure-sdk-for-go#2075

Merged

Conversation

shalinved commented Jun 1, 2018 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR information

Quality of Swagger

Uh oh!

msftclas commented Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AutorestCI commented Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-libraries-for-java

Uh oh!

AutorestCI commented Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-python

Uh oh!

AutorestCI commented Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-node

Uh oh!

AutorestCI commented Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-ruby

Uh oh!

AutorestCI commented Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-go

Uh oh!

lmazuel commented Jun 1, 2018

Uh oh!

shalinved commented Jun 1, 2018

Uh oh!

lmazuel commented Jun 4, 2018

Uh oh!

fearthecowboy commented Jun 5, 2018

Uh oh!

ravbhatnagar left a comment

Choose a reason for hiding this comment

Uh oh!

ravbhatnagar Jun 6, 2018

Choose a reason for hiding this comment

Uh oh!

ravbhatnagar Jun 6, 2018

Choose a reason for hiding this comment

Uh oh!

shalinved Jun 6, 2018

Choose a reason for hiding this comment

Uh oh!

ravbhatnagar Jun 6, 2018

Choose a reason for hiding this comment

Uh oh!

ravbhatnagar commented Jun 6, 2018

Uh oh!

ms-premp commented Jun 7, 2018

Uh oh!

ravbhatnagar commented Jun 12, 2018

Uh oh!

shalinved commented Jun 12, 2018

Uh oh!

shalinved commented Jun 12, 2018

Uh oh!

shalinved commented Jun 13, 2018

Uh oh!

shalinved commented Jun 13, 2018

Uh oh!

ravbhatnagar commented Jun 14, 2018

Uh oh!

lmazuel Jun 14, 2018 • 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!

ms-premp Jun 14, 2018

Choose a reason for hiding this comment

Uh oh!

lmazuel left a comment

Choose a reason for hiding this comment

Uh oh!

jhendrixMSFT commented Jun 19, 2018

Uh oh!

shalinved commented Jun 1, 2018 •

edited by azuresdkci

Loading

msftclas commented Jun 1, 2018 •

edited

Loading

AutorestCI commented Jun 1, 2018 •

edited

Loading

AutorestCI commented Jun 1, 2018 •

edited

Loading

AutorestCI commented Jun 1, 2018 •

edited

Loading

AutorestCI commented Jun 1, 2018 •

edited

Loading

AutorestCI commented Jun 1, 2018 •

edited

Loading

lmazuel Jun 14, 2018 •

edited

Loading