[App Insights/Operational Insights] New Data Plane API versions, bug fixes, and cleanup #3084

alexeldeib · 2018-05-16T00:56:42Z

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.

azuresdkciprbot · 2018-05-16T01:01:30Z

AutoRest linter results for SDK Related Validation Errors/Warnings

These errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns.

File: specification/applicationinsights/data-plane/readme.md

⚠️

0 new Warnings.(0 total)

❌0 new Errors.(0 total)

File: specification/operationalinsights/data-plane/readme.md

⚠️

0 new Warnings.(0 total)

❌0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

azuresdkciprbot · 2018-05-16T01:01:30Z

AutoRest linter results for ARM Related Validation Errors/Warnings

These errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns.

File: specification/applicationinsights/data-plane/readme.md

⚠️

0 new Warnings.(0 total)

❌0 new Errors.(0 total)

File: specification/operationalinsights/data-plane/readme.md

⚠️

0 new Warnings.(0 total)

❌0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

AutorestCI · 2018-05-16T01:06:02Z

Automation for azure-libraries-for-java

Nothing to generate for azure-libraries-for-java

AutorestCI · 2018-05-16T01:15:43Z

Automation for azure-sdk-for-node

Nothing to generate for azure-sdk-for-node

alexeldeib · 2018-05-16T01:19:20Z

Change Summary

New
- App Insights data plane version 2018-04-20
  - Corresponds very closely to existing v1 version
- Operational Insights data plane version 2017-10-01
  - Corresponds closely to existing v1
- Add applications parameter for App Insights
- Add workspaces parameter for Operational Insights
- Add python generation for App Insights
- Add tags to readmes for App Insights and Operational Insights
Changes
- Moved existing v1 version under a /preview folder (previously under root of repo)
- Add x-ms-enum for metricId
- Add x-ms-enum for metricsSegment
- Add x-ms-enum for metricsAggregation
- Add ["count", "unique"] as enum values for metricsAggregation
- Bug: metric validation was too constrained, must be modeled as string *breaking*
- Bug: metric timespan was too constrained as a duration, must be modeled as string *breaking*
- Bug: event type had modelAsString: false. Users can instantiate new values for this in their data model, so we need to modelAsString: true.
- Bug: events timespan was too constrained as duration, must be modeled as string. *breaking*
- Bug: query timespan was too constrained as duration, must be modeled as string. *breaking*
- Fixed some links to external docs (links point to relative locations on an internal site)

AutorestCI · 2018-05-16T01:44:00Z

Automation for azure-sdk-for-python

A PR has been created for you:
Azure/azure-sdk-for-python#2701

AutorestCI · 2018-05-16T02:43:59Z

Automation for azure-sdk-for-go

A PR has been created for you:
Azure/azure-sdk-for-go#1991

marstr · 2018-05-24T17:32:48Z

Howdy @alexeldeib, sorry about the delay here! I'll have a review in by EOD.

alexeldeib · 2018-05-29T20:05:37Z

@marstr Any update here? I realize this is a non-trivial batch of changes, I tried to ease the review effort with the summary. Realistically I could have broken this into a few smaller PRs...20/20 hindsight and all 👓 Let me know if I can do any clean up that would make review easier.

marstr · 2018-05-30T15:08:53Z

Ah sorry @alexeldeib, just dropping balls over here! I'll start reviewing now.

marstr

Nothing egregious really, and if you decide to ignore my comments, I'll approve once you acknowledge them.

In terms of the CI, there is a linter rule that's failing, but it is an ARM specific rule that is not applicable to you. It is your decision if you implement the "operations" endpoint.

marstr · 2018-05-30T15:28:42Z

specification/applicationinsights/data-plane/microsoft.insights/preview/2018-04-20/swagger.json

+  "paths": {
+    "/subscriptions/{subscriptionId}/resourcegroups/{resourceGroupName}/providers/Microsoft.Insights/components/{applicationName}/query": {
+      "post": {
+        "operationId": "Query_Post",


It may be more conventional to have this be "Query_Create", similar to how we name PUTs as "CreateOrUpdate".

This is still a read operation, but maybe something else that doesn't convey a write op? Or else I'd probably prefer to leave it as is

Maybe Query_Execute?

marstr · 2018-05-30T15:29:38Z

specification/applicationinsights/data-plane/microsoft.insights/preview/2018-04-20/swagger.json

+{
+  "swagger": "2.0",
+  "info": {
+    "version": "2018-04-20",


I see here and in your summarizing comment, that you seem to be moving to date strings for your API Version instead of a SemVer-like version string. Given that this is a data-plane library, you're free to use whichever you like, so it surprises me a little that you'd switch. Do you have documentation available that'll help folks navigate the different version formats?

We're actually keeping both, I realize this is a bit funky. We're data plane but accessible via both ARM and a direct endpoint. For the direct endpoint (e.g. api.loganalytics.io), we use our own versioning (e.g. "v1"). In parallel we offer versions using Azure-style date string accessible via management.azure.com.

Generally these are very close/almost identical to their direct counterparts except where parameters differ, e.g. taking the Azure resource ID vs. taking a service-specific GUID. We doc the URL syntax (with versions) for both. Besides the URLs, the only difference is that the direct endpoint supports substantially larger payloads than ARM's limits (which matters, since we often return large DB query results).

marstr · 2018-05-30T16:34:24Z

specification/applicationinsights/data-plane/microsoft.insights/preview/2018-04-20/swagger.json

+    },
+    "/subscriptions/{subscriptionId}/resourcegroups/{resourceGroupName}/providers/Microsoft.Insights/components/{applicationName}/metrics/metadata": {
+      "get": {
+        "operationId": "GetMetricsMetadata",


Given that you broke out "Query" into its own operation group, I'd recommend doing the same for Metric and Event. That would mean that you'd have:

Metric_GetMetadata

Metric_Get

Event_List

Event_Get

Event_GetMetadata (optionally with the OData Suffix as well, based on your judgement.)

This is great -- I was hoping to do some cleanup here but had only played around with /query. Can you elaborate on how this impacts things downstream from swagger? From what I can tell this is used primarily for grouping operations in docs and (maybe in some of the?) SDKs? Still exploring how all the pieces fit together 😄

Ah yeah, for sure! The main difference as you suspect, is about group operations together. In addition to doc generation impacts, the AutoRest modeler actually assigns each operation group its own client. For example, you can see in the Go SDK for the Compute there are separate clients for interacting with VirtualMachines and interacting with a VM Image.

Getting the balance right of not making people new-up clients for everything, but also keeping operations discoverable is a bit of an art. You're really the only one who can tell me if it makes sense to interact with Queries, Metrics, and Events at the same time, or if there's always separation there. 😃

Hmm...So I grouped the operations up and generated the C# and Python SDKs for comparison. I see how the operations group but It looks like they don't get a new client? Using the compute example, here's the C# version. I think for our purposes one client makes sense anyway, but I couldn't replicate what the Go SDK you pointed out was doing in another language -- wanted to clarify the expected behavior?

marstr · 2018-05-30T16:39:26Z

...rationalinsights/data-plane/Microsoft.OperationalInsights/stable/v1/OperationalInsights.json

+      "name": "workspaces",
+      "description": "Comma separated workspace IDs to include in cross-workspace queries.",
+      "in": "query",
+      "collectionFormat": "csv",


While you're at a refactoring with breaking changes, have you thought of making this a JSON array instead of a CSV list? I know that goes beyond Swagger changes, but I believe it'd improve the user experience.

For GET it's a csv in the query string, for POST it's a JSON array in the body. I think that makes sense?

Ah, yeah that totally makes sense! Sorry, I should've looked closer.

marstr · 2018-05-30T16:40:44Z

...operationalinsights/data-plane/Microsoft.OperationalInsights/preview/2017-10-01/swagger.json

+  "paths": {
+    "/subscriptions/{subscriptionId}/resourcegroups/{resourceGroupName}/providers/Microsoft.OperationalInsights/workspaces/{workspaceName}/query": {
+      "post": {
+        "operationId": "Query_Post",


Same comment as in the other API Version, I'd imagine this should be Query_Create.

AutorestCI · 2018-06-07T20:18:46Z

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

alexeldeib · 2018-06-08T18:25:34Z

Besides the operation groupings, let me know if you see anything remaining that sticks out.

I'm also curious about the SDK treatment in the readme. I've been working to tie in with the Python and Nodejs SDKs. I saw you added treatment for Go and OperationalInsights also got Java treatment at some point. I guess i'm trying to ask a process clarification question -- I plan to continue adding SDK support where there are gaps still for these two RPs, but I don't want to step on any toes or duplicate work with SDK teams. For example, are there any known areas I should ignore for now because other teams hook into them, or places I should focus on fleshing out due to a lack of usage by others?

Based on your reply, I might fill in some more SDK generation bits in the readme (e.g., App Insights Java, nodejs and typescript for both). Other than that from my end this looks pretty much good to go!

marstr · 2018-06-12T23:20:16Z

@alexeldeib says:
are there any known areas I should ignore for now because other teams hook into them, or places I should focus on fleshing out due to a lack of usage by others?

I don't think you can step on any toes here! As for the other changes you're talking about, they seem like a good candidate for future PRs.

marstr · 2018-06-12T23:21:21Z

Let me know if you're ready to merge :)

alexeldeib · 2018-06-12T23:24:44Z

Awesome -- ready to merge on my end! 👍

Clean up a few not used fields

alexeldeib and others added 8 commits May 14, 2018 21:29

Proposed specification cleanup

ff2cf1a

Remove 2015-05-01, will not be exposed in this state

23d6239

remove inoperative directive

e149376

parameter location

02b8b16

re-fix directive

80cae83

Add back some operations

3489bdd

new versions

f05ab5f

fix app parameter for v1 direct

99ac4e2

azuresdkci assigned marstr May 16, 2018

azuresdkci requested a review from marstr May 16, 2018 00:57

AutorestCI mentioned this pull request May 16, 2018

[AutoPR applicationinsights/data-plane/operationalinsights/data-plane] [App Insights/Operational Insights] New Data Plane API versions, bug fixes, and cleanup Azure/azure-sdk-for-python#2568

Closed

AutorestCI mentioned this pull request May 16, 2018

[AutoPR operationalinsights/data-plane/applicationinsights/data-plane] [App Insights/Operational Insights] New Data Plane API versions, bug fixes, and cleanup Azure/azure-sdk-for-go#1838

Closed

alexeldeib mentioned this pull request May 25, 2018

Add Log Analytics data plane SDK Azure/azure-sdk-for-python#2557

Merged

marstr requested changes May 30, 2018

View reviewed changes

Regroup operations for client generation, review cleanup.

a017326

AutorestCI mentioned this pull request Jun 7, 2018

[AutoPR operationalinsights/data-plane/applicationinsights/data-plane] [App Insights/Operational Insights] New Data Plane API versions, bug fixes, and cleanup Azure/azure-sdk-for-python#2701

Closed

AutorestCI mentioned this pull request Jun 7, 2018

[AutoPR operationalinsights/data-plane/applicationinsights/data-plane] [App Insights/Operational Insights] New Data Plane API versions, bug fixes, and cleanup Azure/azure-sdk-for-go#1991

Merged

alexeldeib added 2 commits June 7, 2018 13:34

Remove query schema from preview versions

4b3b33e

query operation, readme cleanup

bf6b4b0

marstr approved these changes Jun 12, 2018

View reviewed changes

marstr merged commit 9eef844 into Azure:master Jun 12, 2018

alexeldeib mentioned this pull request Sep 24, 2018

[App Insights] Sync to swagger, bump to 1.0.0-preview-1, rename package *breaking changes* Azure/azure-sdk-for-net#4790

Merged

9 tasks

alexeldeib mentioned this pull request Oct 30, 2018

Add example with nested segmentation, fix number types, remove extraneous produces #4311

Merged

10 tasks

mccleanp pushed a commit that referenced this pull request Mar 23, 2022

Merge pull request #3084 from sureleo/dev/shul/remove-unused-fields

74daac7

Clean up a few not used fields

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[App Insights/Operational Insights] New Data Plane API versions, bug fixes, and cleanup #3084

[App Insights/Operational Insights] New Data Plane API versions, bug fixes, and cleanup #3084

alexeldeib commented May 16, 2018 •

edited

Loading

azuresdkciprbot commented May 16, 2018

azuresdkciprbot commented May 16, 2018

AutorestCI commented May 16, 2018 •

edited

Loading

AutorestCI commented May 16, 2018 •

edited

Loading

alexeldeib commented May 16, 2018

AutorestCI commented May 16, 2018 •

edited

Loading

AutorestCI commented May 16, 2018 •

edited

Loading

marstr commented May 24, 2018

alexeldeib commented May 29, 2018

marstr commented May 30, 2018

marstr left a comment

marstr May 30, 2018

alexeldeib May 30, 2018

marstr May 30, 2018

marstr May 30, 2018

alexeldeib May 30, 2018

marstr May 30, 2018

alexeldeib May 30, 2018

marstr May 30, 2018

alexeldeib Jun 8, 2018

marstr May 30, 2018

alexeldeib May 30, 2018

marstr May 30, 2018

marstr May 30, 2018

AutorestCI commented Jun 7, 2018 •

edited

Loading

alexeldeib commented Jun 8, 2018

marstr commented Jun 12, 2018

marstr commented Jun 12, 2018

alexeldeib commented Jun 12, 2018

[App Insights/Operational Insights] New Data Plane API versions, bug fixes, and cleanup #3084

[App Insights/Operational Insights] New Data Plane API versions, bug fixes, and cleanup #3084

Conversation

alexeldeib commented May 16, 2018 • edited Loading

PR information

Quality of Swagger

azuresdkciprbot commented May 16, 2018

AutoRest linter results for SDK Related Validation Errors/Warnings

azuresdkciprbot commented May 16, 2018

AutoRest linter results for ARM Related Validation Errors/Warnings

AutorestCI commented May 16, 2018 • edited Loading

Automation for azure-libraries-for-java

AutorestCI commented May 16, 2018 • edited Loading

Automation for azure-sdk-for-node

alexeldeib commented May 16, 2018

Change Summary

AutorestCI commented May 16, 2018 • edited Loading

Automation for azure-sdk-for-python

AutorestCI commented May 16, 2018 • edited Loading

Automation for azure-sdk-for-go

marstr commented May 24, 2018

alexeldeib commented May 29, 2018

marstr commented May 30, 2018

marstr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AutorestCI commented Jun 7, 2018 • edited Loading

Automation for azure-sdk-for-ruby

alexeldeib commented Jun 8, 2018

marstr commented Jun 12, 2018

marstr commented Jun 12, 2018

alexeldeib commented Jun 12, 2018

alexeldeib commented May 16, 2018 •

edited

Loading

AutorestCI commented May 16, 2018 •

edited

Loading

AutorestCI commented May 16, 2018 •

edited

Loading

AutorestCI commented May 16, 2018 •

edited

Loading

AutorestCI commented May 16, 2018 •

edited

Loading

AutorestCI commented Jun 7, 2018 •

edited

Loading