[Do not merge] Addition of CRUD operations for Application Insights Components & Synthetic Monitors #988

d3r3kk · 2017-03-03T19:05:23Z

Add the swagger spec for all CRUD operations to do with Components and Synthetic Monitors (web tests).

Spawned from PR "Application Insights: CRUD for a new Instance #757" originally put forth by @tombuildsstuff and worked on by @amarzavery.

Please review and let me know if you have any concerns: @SergeyKanzhelev, @salameer. Any feedback is greatly appreciated.

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.
- Validation errors from the Linter extension for VS Code have all been fixed for this spec. (Note: for large, previously checked in specs, there will likely be many errors shown. Please contact our team so we can set a timeframe for fixing these errors if your PR is not going to address them).
- The spec follows the patterns described in the Swagger good patterns document unless the service API makes this impossible.

…etic Monitors

- Make descriptions consistent with regards to punctuation - Add some missing response codes - Correct a few definition fields for readonly

msftclas · 2017-03-03T19:05:27Z

@d3r3kk,
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

tombuildsstuff · 2017-03-03T19:07:58Z

@d3r3kk this is great - thanks!

d3r3kk · 2017-03-03T19:17:07Z

@tombuildsstuff Please have a look and let me know if there are any problems with what you were attempting to do with the API. Having real-world usage against this API will be very helpful feedback!

- original dev work didn't take into account 'packages' where similar objects are merged during API generation. - Fixed problems with inconsistent common-object defintions

- Revert my definition of the Azure 'Resource' to suit the already-defined swagger specs in the insights management client.

tombuildsstuff · 2017-03-03T20:33:50Z

@d3r3kk thanks for this - to keep you in the loop, I took a quick look but can't generate the Go SDK at this point from the composite swagger right now due to an error:

 $ dotnet AutoRest.dll -Namespace applicationinsights -CodeGenerator Go -Modeler Swagger -Input ~/code/src/github.com/Azure/azure-rest-api-specs/arm-insights/compositeInsightsManagementClient.json
ERROR: Operations API must be implemented for the service.
	Path: ~/code/src/github.com/Azure/azure-rest-api-specs/arm-insights/compositeInsightsManagementClient.json#/paths
WARNING: Parameters "subscriptionId" or "api-version" are referenced but not defined in the parameters section of Service Definition
	Path: ~/code/src/github.com/Azure/azure-rest-api-specs/arm-insights/compositeInsightsManagementClient.json#/parameters
FATAL: Invalid swagger 2.0 specification. Missing version property.
FATAL: Error generating client model: AutoRest.Core.Logging.CodeGenerationException: Invalid swagger 2.0 specification. Missing version property.
   at AutoRest.Swagger.SwaggerModeler.InitializeClientModel() in ~/code/src/github.com/Azure/AutoRest/src/modeler/AutoRest.Swagger/SwaggerModeler.cs:line 226
   at AutoRest.Swagger.SwaggerModeler.Build(ServiceDefinition serviceDefinition) in ~/code/src/github.com/Azure/AutoRest/src/modeler/AutoRest.Swagger/SwaggerModeler.cs:line 93
   at AutoRest.Swagger.SwaggerModeler.Build() in ~/code/src/github.com/Azure/AutoRest/src/modeler/AutoRest.Swagger/SwaggerModeler.cs:line 72
   at AutoRest.Core.AutoRestController.Generate() in ~/code/src/github.com/Azure/AutoRest/src/core/AutoRest.Core/AutoRestController.cs:line 57
ERROR: Error generating client model: AutoRest.Core.Logging.CodeGenerationException: Invalid swagger 2.0 specification. Missing version property.
   at AutoRest.Swagger.SwaggerModeler.InitializeClientModel() in ~/code/src/github.com/Azure/AutoRest/src/modeler/AutoRest.Swagger/SwaggerModeler.cs:line 226
   at AutoRest.Swagger.SwaggerModeler.Build(ServiceDefinition serviceDefinition) in ~/code/src/github.com/Azure/AutoRest/src/modeler/AutoRest.Swagger/SwaggerModeler.cs:line 93
   at AutoRest.Swagger.SwaggerModeler.Build() in ~/code/src/github.com/Azure/AutoRest/src/modeler/AutoRest.Swagger/SwaggerModeler.cs:line 72
   at AutoRest.Core.AutoRestController.Generate() in ~/code/src/github.com/Azure/AutoRest/src/core/AutoRest.Core/AutoRestController.cs:line 57
ERROR: Code generation failed.
ERROR: AutoRest.dll -Namespace applicationinsights -CodeGenerator Go -Modeler Swagger -Input ~/code/src/github.com/Azure/azure-rest-api-specs/arm-insights/compositeInsightsManagementClient.json

I noticed some errors in Travis too - but that the Go generator isn't hooked up to that yet?

Update: I've just noticed you've pushed some more commits - I'll check back later on - thanks again :)

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

arm-insights/2015-05-01/swagger/ApplicationInsights_SyntheticMonitor.json

+      "in": "query",
+      "required": true,
+      "type": "string",
+      "description": "Client API version."


All documents of composite Swagger need to agree about parameters defined on the global scope 100%. In this case, AutoRest complains about a mismatching description ;-) In another document in here I see (note capitalization):

"ApiVersionParameter": { "name": "api-version", "in": "query", "required": true, "type": "string", "description": "Client Api Version." },

Please make sure all documents use precisely the same ApiVersionParameter definition.

@olydis Fixed in my latest commit - I was unfortunately using the Nuget AutoRest package which is currently at v0.17.0 and did not report this error. I've since updated to use the NPM autorest package which is at 1.0.1 and have corrected the problem.

@tombuildsstuff I've just now built the Go API and it succeeded without warnings.

amarzavery · 2017-03-03T22:03:28Z

@tombuildsstuff - Those are validation errors and not codegen errors. Both use the same logger. If an error was logged ever the final exit code is 1 and you see that error message. We are fixing that part.
However, the code should still be generated. Please check the output directory, you should see the generated client in there. Sorry for the bad experience.

amarzavery · 2017-03-03T22:14:41Z

arm-insights/2015-05-01/swagger/ApplicationInsights_ComponentClient.json

+            "schema": {
+              "$ref": "#/definitions/ApplicationInsightsComponentListResult"
+            },
+            "examples": {


instead of providing example in the spec, can you please provide them using the "x-ms-examples" extension.
Take a look at the RedisCache swagger spec as an example over here.

Complete, have a look and let me know - tested using 'npm test -- test/model.js' and got that running cleanly for the Components and SyntheticMonitor specs I've added only.

- use x-ms-examples format for response examples rather than the inline version

- users will want an exception for these cases too

gucalder · 2017-03-03T23:58:03Z

This change is being applied to the arm-insights folder, which is not the AppInsights folder.

hovsepm · 2017-03-04T00:03:46Z

@d3r3kk, @amarzavery, @olydis

This PR targets wrong folder. Please re-target it to the correct folder per @gucalder comment.

- Correct errors found by using the model test definition

- PR request - use the x-ms-examples method of defining examples for synthetic monitors - Remove 404 codes, we want users to get exceptions for those - Ensure the test/model.js tests pass for the Synthetic Monitor examples

d3r3kk · 2017-03-04T01:12:08Z

@gucalder, @hovsepm, I was under the understanding that Insights includes Application Inisghts. Indeed, the 'Alerts' API that is found under ../arm-insights/ is provided by the same web service that provides Component and Synthetic Monitor APIs. I can create an arm-appinsights folder though, I have no problem doing so.

d3r3kk · 2017-03-04T01:13:18Z

@amarzavery I've changed all examples to using the x-ms-examples pattern.

- Request of PR members, this is the new place for the monitor APIs to reside, specifically for Microsoft.insights. - Changed name of files to coincide with the pattern in this folder - Changed title of the Component and Synthetic Monitor APIs to match the rest.

amarzavery · 2017-03-08T17:16:59Z

@d3r3kk @hovsepm
I think there is some miscommunication over here. When @tombuildsstuff (our customer) sent the initial PR I was trying to find the right service team internally who can review the PR as they would know the APIs in depth. In that process, I approached @gucalder and his manager @andyshen since the provider namespace was Microsoft.Insights. They told me that even though ApplicationInsights and Insights share the same provider namespace (/subscriptions/{subscriptionId}/providers/ Microsoft.Insights/ ...), they are completely different services with nothing in common. To avoid this confusion Insights is changing (just the folder name and client library (package name) to Monitoring. I think the swagger spec in this PR should reside in a separate folder named arm-applicationinsights.

Does that make sense?

amarzavery · 2017-03-08T17:19:58Z

@d3r3kk - I would suggest syncing internally with @hovsepm @gucalder and @andyshen and then make the final push. This will help us reach to the logical confusion faster.

d3r3kk · 2017-03-08T17:22:50Z

@amarzavery I've actually been chatting with @hovsepm and @gucalder in an email thread I'll add you to this morning. Apologies for any confusion I may have caused here!

I am trying to determine the best possible outcome for the customer (components, synthetic monitors, and alerts, are all coupled and will be used in tandem by our users when they use application insights APIs).

d3r3kk · 2017-03-21T16:04:28Z

Awesome feedback, thanks @amarzavery! Sorry I haven't responded for 5 days, I've since moved onto other efforts for my team, but will revisit this PR by this coming Friday, 24th of March.

- Rename SyntheticMonitor to WebTests to conform to spec - Method names have been normalized - Required "Patch" operation added - Metadata operation added in new API file (aiOperations) - Various minor fixes in type names and definitions

- Every field that should be a default value has been switched to an enum - Each field has a 'default' value - Each field is 'modelAsString' to ensure non-accounted-for values are still allowed during deserialization

- Incorrect casing of filenames was failing the syntax checks. - Errant comma in one example also failed the syntax check

d3r3kk · 2017-03-28T21:24:22Z

Thanks for your patience everyone. I've posted my changes requested by @amarzavery along with some new example files as well.

Please review and let me know if there is any further work to do to ensure the quality of our spec is inline with the repo.

Thanks!

amarzavery · 2017-03-29T17:54:51Z

@d3r3kk - The model validator found issues. Please take a look at the Travis CI log over here https://travis-ci.org/Azure/azure-rest-api-specs/jobs/216112575#L1042

amarzavery · 2017-03-29T17:57:25Z

test/linter.js

-  // swaggersToProcess = swaggersToProcess.filter(function(item) {
-  //   return (item.match(/.arm-containerregistry.*2017-03-01.*/ig) !== null);
-  // });
+  swaggersToProcess = swaggersToProcess.filter(function(item) {


Please revert this change.

amarzavery · 2017-03-29T17:58:57Z

arm-appinsights/2015-05-01/swagger/webTests_API.json

          "type": "integer",
          "format": "int32",
-          "description": "Seconds until this WebTest will timeout and fail."
+          "description": "Seconds until this WebTest will timeout and fail. Default valu is 30.",


Default valu is 30.", -> missing 'e' in value

amarzavery · 2017-03-29T18:03:21Z

There is an enum mismatch in the examples and what is specified in swagger. Values you specify in the enum array in the swagger spec are case sensitive. This also affects deserialization in the generated sdks. Make sure that the casing of all the enum values matches exactly what is being sent on the wire.
x-ms-example "webTestUpdate" in operation "WebTests_CreateOrUpdate". 'OBJECT_MISSING_REQUIRED_PROPERTY', -> 'SyntheticMonitorId'

- mistakenly added local commit limiting range of linter tests

- Specify the enum-type values in the Create example - Alter the values in the responses to reflect the accepted values from our spec. - NOTE: There are other values allowed, hence our modelAsString = true attribute.

d3r3kk · 2017-03-29T20:20:12Z

@amarzavery I believe I've rectified the issues with examples / typos. Apologies for checking in the linter test.

amarzavery · 2017-03-29T21:51:53Z

Hey @d3r3kk the travis ci shows that there are still a bunch of model validation errors:
https://travis-ci.org/Azure/azure-rest-api-specs/jobs/216467585#L1046

Can you please fix all of those?

d3r3kk · 2017-03-30T16:17:30Z

Done.

What about the insights/operations link. I see that I fail in the linter script for those for the two main spec files, but I do not fail in the single file I included that in.

It seems I have to include that path in each of my swagger spec files to pass the linter. Would that be preferable to the solution I've devised?

amarzavery · 2017-04-04T18:39:31Z

@d3r3kk - The linter is run on each swagger spec separately. It has no ideas that the swagger is a part of the composite swagger. Hence please ignore the operations API Implementation error in other specs. If you have included the API in one of the swagger specs then you are good to go.

AutorestCI · 2017-04-04T18:44:57Z

No modification for Python

AutorestCI · 2017-04-04T18:44:59Z

No modification for Ruby

amarzavery · 2017-04-04T18:45:08Z

Thanks @d3r3kk for authoring the swagger and incorporating all the feedback.

@tombuildsstuff - Sorry for the delay. But you should be able to use this (applicationInsights) swagger and the clients generated from this swagger. Please feel free to file issues if you see any problems. Thanks for the initial PR with swagger. That helped us deliver this today.

AutorestCI · 2017-04-04T18:45:17Z

No modification for NodeJS

salameer · 2017-04-04T21:09:38Z

@d3r3kk Thanks again, @tombuildsstuff, we'll make sure that this get's out into our next Go SDK release. adding @marstr for visibility

d3r3kk and others added 3 commits March 3, 2017 10:54

Add swagger specification for Application Insights Components & Synth…

d3a283a

…etic Monitors

Correcting some descriptions and adding further documentation

68edd50

Last cleanup pass before submitting to Github

c5991c9

- Make descriptions consistent with regards to punctuation - Add some missing response codes - Correct a few definition fields for readonly

msftclas added the cla-not-required label Mar 3, 2017

azuresdkci assigned olydis Mar 3, 2017

d3r3kk added 2 commits March 3, 2017 12:03

Correct swagger spec for packaging

a720d68

- original dev work didn't take into account 'packages' where similar objects are merged during API generation. - Fixed problems with inconsistent common-object defintions

Correct swagger spec for packaging once again

d305f0f

- Revert my definition of the Azure 'Resource' to suit the already-defined swagger specs in the insights management client.

olydis reviewed Mar 3, 2017

View reviewed changes

olydis added the In-Review label Mar 3, 2017

Make ApiVersionParameter consistent

cfcfe0f

amarzavery suggested changes Mar 3, 2017

View reviewed changes

d3r3kk added 2 commits March 3, 2017 15:30

PR Request: Add example json for Component.

7db4824

- use x-ms-examples format for response examples rather than the inline version

Remove 404 expected responses from components

66d9c4a

- users will want an exception for these cases too

hovsepm changed the title ~~Addition of CRUD operations for Application Insights Components & Synthetic Monitors~~ [Do not merge] Addition of CRUD operations for Application Insights Components & Synthetic Monitors Mar 4, 2017

d3r3kk added 2 commits March 3, 2017 16:08

Correct example JSON for AI Components

6d92719

- Correct errors found by using the model test definition

Correct SyntheticMonitor examples

5b0bb80

- PR request - use the x-ms-examples method of defining examples for synthetic monitors - Remove 404 codes, we want users to get exceptions for those - Ensure the test/model.js tests pass for the Synthetic Monitor examples

Remove AI Component & Synthetic Monitor API from arm-insights folder

13a189d

veronicagg removed their assignment Mar 17, 2017

d3r3kk added 5 commits March 28, 2017 12:06

PR Request fixes

804ba20

- Rename SyntheticMonitor to WebTests to conform to spec - Method names have been normalized - Required "Patch" operation added - Metadata operation added in new API file (aiOperations) - Various minor fixes in type names and definitions

Correct required fields in WebTestProperties

a0e297d

Default value fields corrections

afd4217

- Every field that should be a default value has been switched to an enum - Each field has a 'default' value - Each field is 'modelAsString' to ensure non-accounted-for values are still allowed during deserialization

Correct WebTest example JSON

7876a19

- Incorrect casing of filenames was failing the syntax checks. - Errant comma in one example also failed the syntax check

Fix linter error with aiOperations.json

7f34b8b

amarzavery reviewed Mar 29, 2017

View reviewed changes

d3r3kk added 4 commits March 29, 2017 11:05

Revert test/linter.js

6f10918

- mistakenly added local commit limiting range of linter tests

Fix misspelled word in description

a05877e

Correct mismatch of required values in examples

300d68e

Correct enum values in example

95818c5

- Specify the enum-type values in the Create example - Alter the values in the responses to reflect the accepted values from our spec. - NOTE: There are other values allowed, hence our modelAsString = true attribute.

PR Fixes example values corrected

64b6271

amarzavery approved these changes Apr 4, 2017

View reviewed changes

amarzavery merged commit f552cc9 into Azure:master Apr 4, 2017

d3r3kk deleted the d3r3kk-appinsights-crud branch May 25, 2017 14:55

[Do not merge] Addition of CRUD operations for Application Insights Components & Synthetic Monitors #988

[Do not merge] Addition of CRUD operations for Application Insights Components & Synthetic Monitors #988

Uh oh!

Conversation

d3r3kk commented Mar 3, 2017 • 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 Mar 3, 2017

Uh oh!

tombuildsstuff commented Mar 3, 2017

Uh oh!

d3r3kk commented Mar 3, 2017

Uh oh!

tombuildsstuff commented Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olydis Mar 3, 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!

d3r3kk Mar 3, 2017

Choose a reason for hiding this comment

Uh oh!

amarzavery commented Mar 3, 2017

Uh oh!

amarzavery Mar 3, 2017

Choose a reason for hiding this comment

Uh oh!

d3r3kk Mar 3, 2017

Choose a reason for hiding this comment

Uh oh!

d3r3kk Mar 4, 2017

Choose a reason for hiding this comment

Uh oh!

gucalder commented Mar 3, 2017

Uh oh!

hovsepm commented Mar 4, 2017

Uh oh!

d3r3kk commented Mar 4, 2017

Uh oh!

d3r3kk commented Mar 4, 2017

Uh oh!

amarzavery commented Mar 8, 2017

Uh oh!

amarzavery commented Mar 8, 2017

Uh oh!

d3r3kk commented Mar 8, 2017

Uh oh!

d3r3kk commented Mar 21, 2017

Uh oh!

d3r3kk commented Mar 28, 2017

Uh oh!

amarzavery commented Mar 29, 2017

Uh oh!

amarzavery Mar 29, 2017

Choose a reason for hiding this comment

Uh oh!

amarzavery Mar 29, 2017

Choose a reason for hiding this comment

Uh oh!

amarzavery commented Mar 29, 2017

Uh oh!

d3r3kk commented Mar 29, 2017

Uh oh!

amarzavery commented Mar 29, 2017

Uh oh!

d3r3kk commented Mar 30, 2017

Uh oh!

amarzavery commented Apr 4, 2017

Uh oh!

AutorestCI commented Apr 4, 2017

Uh oh!

AutorestCI commented Apr 4, 2017

Uh oh!

amarzavery commented Apr 4, 2017

Uh oh!

AutorestCI commented Apr 4, 2017

Uh oh!

d3r3kk commented Mar 3, 2017 •

edited by azuresdkci

Loading

tombuildsstuff commented Mar 3, 2017 •

edited

Loading

olydis Mar 3, 2017 •

edited

Loading