Application Insights: Add Analytics Items API by yoramsinger · Pull Request #2608 · Azure/azure-rest-api-specs

yoramsinger · 2018-03-07T01:09:39Z

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.

AutorestCI · 2018-03-07T01:10:57Z

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#2691

AutorestCI · 2018-03-07T01:12:14Z

Automation for azure-sdk-for-python

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-python#2022

AutorestCI · 2018-03-07T01:13:39Z

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#1554

olydis · 2018-03-07T03:28:09Z

@ravbhatnagar new Swagger

ravbhatnagar · 2018-03-09T08:49:10Z

@yoramsinger - Are these existing APIs in the service that you are adding to swagger now?

yoramsinger · 2018-03-09T11:31:36Z

Yes. Existing API.

ravbhatnagar · 2018-03-16T02:52:09Z

Signing off from ARM since there are APIs in prod for a while

olydis · 2018-03-16T21:32:00Z

...cationinsights/resource-manager/microsoft.insights/stable/2015-05-01/analyticsItems_API.json

+        "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/microsoft.insights/components/{resourceName}/{scopePath}": {
+            "get": {
+                "description": "Gets a list of Analytics Items defined within an Application Insights component.",
+                "operationId": "AnalyticsItems_List",


please rename to AnalyticsItem_List (operation group names should be consistent) 🙂

olydis · 2018-03-16T21:33:33Z

...cationinsights/resource-manager/microsoft.insights/stable/2015-05-01/analyticsItems_API.json

+    },
+    "definitions": {
+        "ApplicationInsightsComponentAnalyticsItem": {
+            "description": "Properties that define an Analytics item that is associated to an Application Insights component.",


please add "type": "object"

olydis · 2018-03-16T21:35:40Z

...cationinsights/resource-manager/microsoft.insights/stable/2015-05-01/analyticsItems_API.json

+                    ],
+                    "x-ms-enum": {
+                        "name": "ItemType",
+                        "modelAsString": false


please set this to true and remove either the lower or upper case values from the enum list (I assume your service accepts both spellings? If a user sets with specific spelling, will the service return with same spelling or normalize internally? If it normalizes, please choose that spelling here for consistency)

olydis · 2018-03-16T21:36:53Z

...cationinsights/resource-manager/microsoft.insights/stable/2015-05-01/analyticsItems_API.json

+                    ],
+                    "x-ms-enum": {
+                        "name": "ItemScope",
+                        "modelAsString": false


this enum sounds as if it could inherently change at some point in time (e.g. future api version). Please set modelAsString to true in such cases as otherwise any such change would be a breaking change 🙂

olydis · 2018-03-16T21:37:51Z

...cationinsights/resource-manager/microsoft.insights/stable/2015-05-01/analyticsItems_API.json

+                    "type": "string",
+                    "description": "Internally assigned unique id of the item definition."
+                },
+                "Name": {


since you're an existing service it's not unlikely but: Just wanna confirm, is PascalCasing really what your service sends over the wire?

Unfortunately yes, this is how it was defined.

olydis · 2018-03-16T21:40:43Z

...cationinsights/resource-manager/microsoft.insights/stable/2015-05-01/analyticsItems_API.json

+                    "readOnly": true
+                },
+                "Properties": {
+                    "properties": {


please pull this out into its own entry in schemas - that way you can control the name chosen during code generation (as is, autorest would pick some name which is usually horrible).

Also, add type: object to the schema 😉

olydis · 2018-03-16T21:41:03Z

...cationinsights/resource-manager/microsoft.insights/stable/2015-05-01/analyticsItems_API.json

+                "Properties": {
+                    "properties": {
+                        "functionAlias": {
+                        "type": "string",


nit: please format the entire file in the end so indentation is consistent 🙂

olydis · 2018-03-16T21:41:43Z

...cationinsights/resource-manager/microsoft.insights/stable/2015-05-01/analyticsItems_API.json

+            ],
+            "x-ms-enum": {
+                "name": "ItemScopePath",
+                "modelAsString": false


also consider changing to true, see previous comment regarding breaking changes

olydis · 2018-03-16T21:41:47Z

...cationinsights/resource-manager/microsoft.insights/stable/2015-05-01/analyticsItems_API.json

+            ],
+            "x-ms-enum": {
+                "name": "ItemScope",
+                "modelAsString": false


also consider changing to true, see previous comment regarding breaking changes

olydis · 2018-03-16T21:42:22Z

...cationinsights/resource-manager/microsoft.insights/stable/2015-05-01/analyticsItems_API.json

+            ],
+            "x-ms-enum": {
+                "name": "ItemType",
+                "modelAsString": false


also consider changing to true, see previous comment regarding breaking changes

AutorestCI · 2018-03-22T12:57:06Z

Automation for azure-libraries-for-java

Encountered a Subprocess error: (azure-libraries-for-java)

Command: ['/usr/local/bin/autorest', '/tmp/tmpxa8g74qr/rest/specification/applicationinsights/resource-manager/readme.md', '--azure-libraries-for-java-folder=/tmp/tmpxa8g74qr/sdk', '--fluent', '--java', '--multiapi', '--verbose']
Finished with return code 1
and output:

AutoRest code generation utility [version: 2.0.4262; node: v7.10.1]
(C) 2018 Microsoft Corporation.
https://aka.ms/autorest
   Loading AutoRest core      '/root/.autorest/@microsoft.azure_autorest-core@2.0.4272/node_modules/@microsoft.azure/autorest-core/dist' (2.0.4272)
   Loading AutoRest extension '@microsoft.azure/autorest.java' (~2.1.32->2.1.49)
   Loading AutoRest extension '@microsoft.azure/autorest.modeler' (2.3.38->2.3.38)
FATAL: System.Exception: Duplicate File Generation: src/main/java/com/microsoft/azure/management/applicationinsights/implementation/FavoritesInner.java
   at AutoRest.Core.CodeGenerator.<Write>d__13.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at AutoRest.Core.CodeGenerator.<Write>d__12.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at AutoRest.Java.Azure.Fluent.CodeGeneratorJvaf.<Generate>d__6.MoveNext() in C:\Users\autorest-ci\AppData\Local\Temp\2\PUBLISHedxhk\164_20180307T193002\autorest.java\src\azurefluent\CodeGeneratorJvaf.cs:line 57
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at AutoRest.Java.Program.<ProcessInternal>d__3.MoveNext() in C:\Users\autorest-ci\AppData\Local\Temp\2\PUBLISHedxhk\164_20180307T193002\autorest.java\src\Program.cs:line 115
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at NewPlugin.<Process>d__15.MoveNext()
FATAL: java/generate - FAILED
FATAL: Error: Plugin java reported failure.
Process() cancelled due to exception : Plugin java reported failure.
  Error: Plugin java reported failure.

yoramsinger · 2018-03-22T13:00:12Z

I updated per your review. Please verify, thanks.

olydis

looking good overall, some minor things CI tooling found

olydis · 2018-03-22T18:21:41Z

...cationinsights/resource-manager/microsoft.insights/stable/2015-05-01/analyticsItems_API.json

+                    "description": "Enum indicating the type of the Analytics item.",
+                    "type": "string",
+                    "enum": [
+                        "Query",


Your example AnalyticsItemList, AnalyticsItemGet and AnalyticsItemPut disagree with spelling here (seem to be lowercase) - please adjust either examples or Swagger, whichever is wrong 🙂

olydis · 2018-03-22T18:24:20Z

...cationinsights/resource-manager/microsoft.insights/stable/2015-05-01/analyticsItems_API.json

+            "default": "None",
+            "enum": [
+                "None",
+                "Query",


see spelling comment on other instance of this enum

...however, there is another issue: this enum has None has an additional value. This is a problem since both enums are named ItemType, so code-gen would not know what to do. Please either use the same list of values for both enums (if that makes any sense) or choose different names for the two enums - both may be non-ideal solutions, I'd be happy to discuss more

Unfortunately the API was not designed well. I'll use 2 separate enums here.

olydis · 2018-03-30T18:10:42Z

@yoramsinger please address the outstanding issues; this PR will otherwise be closed due to inactivity (but can reopened at any time, once you are ready to move forward)

yoramsinger · 2018-04-02T12:55:38Z

I think I fixed the mentioned issues. Hope it works this time.

olydis · 2018-04-02T18:22:48Z

@yoramsinger Tiny bug: ItemTypeParameter still has a PascalCased default value of None 🙂

olydis

looking good, thanks for the fix!

jhendrixMSFT · 2018-04-11T16:52:34Z

@AutorestCI regenerate azure-sdk-for-go

azuresdkciprbot · 2018-04-12T22:51:36Z

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/applicationinsights/resource-manager/readme.md
Before the PR: Warning(s): 0 Error(s): 0
After the PR: Warning(s): 0 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

yoramsinger added 5 commits March 6, 2018 10:11

Application Insights: Add Analytics Items API

485ad79

Align with AI style

ca68988

Bug fixes

9650f82

Fix bug

d98e53c

Bug fix

fb9697e

azuresdkci assigned olydis Mar 7, 2018

olydis added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Mar 7, 2018

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 Mar 16, 2018

olydis suggested changes Mar 16, 2018

View reviewed changes

olydis added Reviewed-FeedbackProvided Reviewed-ChangesRequested <valid label in PR review process>add this label when assignee request changes after review and removed Reviewed-FeedbackProvided labels Mar 20, 2018

PR fixes

19bab04

Merge branch 'master' into master

6d0ee48

olydis approved these changes Mar 22, 2018

View reviewed changes

olydis added In-Review and removed Reviewed-ChangesRequested <valid label in PR review process>add this label when assignee request changes after review labels Mar 22, 2018

olydis suggested changes Mar 22, 2018

View reviewed changes

olydis added Reviewed-FeedbackProvided and removed In-Review labels Mar 22, 2018

Fix issues

52377b2

yoramsinger and others added 2 commits April 9, 2018 09:37

Typo

1968e74

Merge branch 'master' into master

45986f5

olydis approved these changes Apr 9, 2018

View reviewed changes

olydis merged commit 98d88ad into Azure:master Apr 9, 2018

AutorestCI mentioned this pull request Apr 11, 2018

[AutoPR applicationinsights/resource-manager] Application Insights: Add Analytics Items API Azure/azure-sdk-for-go#1553

Merged

Comments

Conversation

yoramsinger commented Mar 7, 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!

AutorestCI commented Mar 7, 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 Mar 7, 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 Mar 7, 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!

olydis commented Mar 7, 2018

Uh oh!

ravbhatnagar commented Mar 9, 2018

Uh oh!

yoramsinger commented Mar 9, 2018

Uh oh!

ravbhatnagar commented Mar 16, 2018

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!

AutorestCI commented Mar 22, 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!

yoramsinger commented Mar 22, 2018

Uh oh!

olydis 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!

olydis commented Mar 30, 2018

Uh oh!

yoramsinger commented Apr 2, 2018

Uh oh!

olydis commented Apr 2, 2018

Uh oh!

olydis left a comment

Choose a reason for hiding this comment

Uh oh!

jhendrixMSFT commented Apr 11, 2018

Uh oh!

azuresdkciprbot commented Apr 12, 2018

Uh oh!

Reviewers

yoramsinger commented Mar 7, 2018 •

edited by azuresdkci

Loading

AutorestCI commented Mar 7, 2018 •

edited

Loading

AutorestCI commented Mar 7, 2018 •

edited

Loading

AutorestCI commented Mar 7, 2018 •

edited

Loading

AutorestCI commented Mar 22, 2018 •

edited

Loading