Azure Search: Adding Service Statistics API by natinimni · Pull Request #2773 · Azure/azure-rest-api-specs

natinimni · 2018-03-29T18:00:17Z

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-29T18:00:41Z

Automation for azure-sdk-for-go

Nothing to generate for azure-sdk-for-go

AutorestCI · 2018-03-29T18:01:05Z

Automation for azure-sdk-for-node

Nothing to generate for azure-sdk-for-node

AutorestCI · 2018-03-29T18:01:05Z

Automation for azure-sdk-for-python

Nothing to generate for azure-sdk-for-python

AutorestCI · 2018-03-29T18:01:06Z

Automation for azure-libraries-for-java

Nothing to generate for azure-libraries-for-java

natinimni · 2018-03-29T18:02:45Z

FYI @brjohnstmsft

brjohnstmsft

Let's make sure we're using x-nullable in the right places. Otherwise the changes look good to me.

brjohnstmsft · 2018-03-29T20:23:41Z

...arch/data-plane/Microsoft.Azure.Search.Service/preview/2016-09-01-preview/searchservice.json

+          "description": "Total number of synonym maps."
+        },
+      },
+      "description": "Represents a service level resource counters and quotas."


nit: "Represents a service level..." -> "Represents service-level..."

brjohnstmsft · 2018-03-29T20:27:36Z

...arch/data-plane/Microsoft.Azure.Search.Service/preview/2016-09-01-preview/searchservice.json

+        "maxFieldsPerIndex": {
+          "type": "integer",
+          "format": "int32",
+          "x-nullable": true,


Can this really be null?

All the limits could technically be null and while we could decide to prevent that by populating default values at the service side, I don't think we should.

If all the limits could be null, then what about maxIndexerRunTime? Shouldn't it be x-nullable too for consistency?

Are the limits null, or are there no limits? In general, if possible, it is preferable to not have semantically meaningful null values in your REST API.

In this case, what would be the difference in meaning between a null value being sent back from the service, and no maxFieldsPerIndex attribute being included in the response? If there isn't one, my suggestion to not send the key/value pair rather than sending a null value.

Limits may be null if they don't apply, so I'm keeping them nullable, but I did take your suggestion for counters and removed the 'nullable' from resourceCounter.count.

@johanste We discussed offline and I agree with Nati that the intent here is to model limits that don't apply ("unlimited") as null, or a missing property. They both end up the same in the client programming model (null property). I don't know offhand whether our REST API will just omit properties with null values, but in either case changing that behavior now would affect our REST API globally, so it's not something we're likely to do.

Slight correction; the client side programming model will depend on which SDK you are using (if any). The swagger document describes the REST API on the network layer. Anybody who does not use our SDKs (or who are using the "raw" layer of the SDK - the one that doesn't deserialize responses into object models) will be exposed to the "value is null" vs. "key/value is missing" difference. Thus the general recommendation to stick to one way to express missing value (don't send key/value) unless there actually is a difference.

If you could double-check what the service actually does (quite a few will suppress/not send null values) and have the swagger express that, it would be great.

I have corrected the server side behavior just yesterday, to reflect the following changes:
For ‘Counters’ – ‘usage’ property could never be null. If a certain counter is not applicable (although we don’t have that notion in the real world) we won’t send the counter at all.
For ‘Limits’ – limits with null values are not denoting 'unlimited', but rather the irrelevancy of this limit - this is because the way we use OData, which we are unlikely to change.

These behaviors are already reflected by this PR, so no additional commits are required to realize them.

brjohnstmsft · 2018-03-29T20:28:15Z

...arch/data-plane/Microsoft.Azure.Search.Service/preview/2016-09-01-preview/searchservice.json

+        "maxFileExtractionSize": {
+          "type": "integer",
+          "format": "int64",
+          "x-nullable": true,


Same question... Will the REST API ever return null for any of these limits?

brjohnstmsft · 2018-03-29T20:31:39Z

specification/search/data-plane/Microsoft.Azure.Search.Service/readme.md

  clear-output-folder: true
  output-folder: $(csharp-sdks-folder)/Search/DataPlane/Microsoft.Azure.Search.Service/Generated
+
+directive: 


nit: Let's leave a TODO here to remove this workaround when AutoRest fixes the root cause of the issue. Otherwise, updating this becomes an extra maintenance task whenever we want to add a new resource (e.g. -- skillsets).

@sergey-shandar signoff on that part 👍

brjohnstmsft · 2018-03-29T20:36:02Z

@natinimni It looks like there's a missing or extra character in the spec file:


>>>> Number of swaggers found in this PR: 1

>>>>> Files deleted in this PR are as follows:

[]

error:  Error: Unable to read the content or execute "JSON.parse()" on the content of file "specification/search/data-plane/Microsoft.Azure.Search.Service/preview/2016-09-01-preview/searchservice.json". The error is:

SyntaxError: Unexpected token } in JSON at position 140440

    at Object.parseJson (/home/travis/build/Azure/azure-rest-api-specs/node_modules/oav/lib/util/utils.js:78:15)

    at SpecValidator.initialize (/home/travis/build/Azure/azure-rest-api-specs/node_modules/oav/lib/validators/specValidator.js:88:20)

    at Object.validateSpec (/home/travis/build/Azure/azure-rest-api-specs/node_modules/oav/lib/validate.js:73:20)

    at Context.<anonymous> (/home/travis/build/Azure/azure-rest-api-specs/test/semantic.js:19:44)

    at callFn (/home/travis/build/Azure/azure-rest-api-specs/node_modules/mocha/lib/runnable.js:378:21)

    at Test.Runnable.run (/home/travis/build/Azure/azure-rest-api-specs/node_modules/mocha/lib/runnable.js:370:7)

    at Runner.runTest (/home/travis/build/Azure/azure-rest-api-specs/node_modules/mocha/lib/runner.js:442:10)

    at /home/travis/build/Azure/azure-rest-api-specs/node_modules/mocha/lib/runner.js:560:12

    at next (/home/travis/build/Azure/azure-rest-api-specs/node_modules/mocha/lib/runner.js:356:14)

    at /home/travis/build/Azure/azure-rest-api-specs/node_modules/mocha/lib/runner.js:366:7

    at next (/home/travis/build/Azure/azure-rest-api-specs/node_modules/mocha/lib/runner.js:290:14)

    at Immediate.<anonymous> (/home/travis/build/Azure/azure-rest-api-specs/node_modules/mocha/lib/runner.js:334:5)

    at runCallback (timers.js:794:20)

    at tryOnImmediate (timers.js:752:5)

    at processImmediate [as _immediateCallback] (timers.js:729:5)

error: RESOLVE_SPEC_ERROR: Unable to read the content or execute "JSON.parse()" on the content of file "specification/search/data-plane/Microsoft.Azure.Search.Service/preview/2016-09-01-preview/searchservice.json". The error is:

SyntaxError: Unexpected token } in JSON at position 140440.

error: Error: Unable to read the content or execute "JSON.parse()" on the content of file "specification/search/data-plane/Microsoft.Azure.Search.Service/preview/2016-09-01-preview/searchservice.json". The error is:

SyntaxError: Unexpected token } in JSON at position 140440

    at Object.parseJson (/home/travis/build/Azure/azure-rest-api-specs/node_modules/oav/lib/util/utils.js:78:15)

    at SpecValidator.initialize (/home/travis/build/Azure/azure-rest-api-specs/node_modules/oav/lib/validators/specValidator.js:88:20)

    at Object.validateSpec (/home/travis/build/Azure/azure-rest-api-specs/node_modules/oav/lib/validate.js:73:20)

    at Context.<anonymous> (/home/travis/build/Azure/azure-rest-api-specs/test/semantic.js:19:44)

    at callFn (/home/travis/build/Azure/azure-rest-api-specs/node_modules/mocha/lib/runnable.js:378:21)

    at Test.Runnable.run (/home/travis/build/Azure/azure-rest-api-specs/node_modules/mocha/lib/runnable.js:370:7)

    at Runner.runTest (/home/travis/build/Azure/azure-rest-api-specs/node_modules/mocha/lib/runner.js:442:10)

    at /home/travis/build/Azure/azure-rest-api-specs/node_modules/mocha/lib/runner.js:560:12

    at next (/home/travis/build/Azure/azure-rest-api-specs/node_modules/mocha/lib/runner.js:356:14)

    at /home/travis/build/Azure/azure-rest-api-specs/node_modules/mocha/lib/runner.js:366:7

    at next (/home/travis/build/Azure/azure-rest-api-specs/node_modules/mocha/lib/runner.js:290:14)

    at Immediate.<anonymous> (/home/travis/build/Azure/azure-rest-api-specs/node_modules/mocha/lib/runner.js:334:5)

    at runCallback (timers.js:794:20)

    at tryOnImmediate (timers.js:752:5)

    at processImmediate [as _immediateCallback] (timers.js:729:5)

error:  

{ code: 'RESOLVE_SPEC_ERROR',

  id: 'OAV102',

  message: 'Unable to read the content or execute "JSON.parse()" on the content of file "specification/search/data-plane/Microsoft.Azure.Search.Service/preview/2016-09-01-preview/searchservice.json". The error is:\nSyntaxError: Unexpected token } in JSON at position 140440',

  innerErrors: [ {} ] }

{ code: 'RESOLVE_SPEC_ERROR',

  id: 'OAV102',

  message: 'Unable to read the content or execute "JSON.parse()" on the content of file "specification/search/data-plane/Microsoft.Azure.Search.Service/preview/2016-09-01-preview/searchservice.json". The error is:\nSyntaxError: Unexpected token } in JSON at position 140440',

  innerErrors: 

   [ Error: Unable to read the content or execute "JSON.parse()" on the content of file "specification/search/data-plane/Microsoft.Azure.Search.Service/preview/2016-09-01-preview/searchservice.json". The error is:

SyntaxError: Unexpected token } in JSON at position 140440

    at Object.parseJson (/home/travis/build/Azure/azure-rest-api-specs/node_modules/oav/lib/util/utils.js:78:15)

    at SpecValidator.initialize (/home/travis/build/Azure/azure-rest-api-specs/node_modules/oav/lib/validators/specValidator.js:88:20)

    at Object.validateSpec (/home/travis/build/Azure/azure-rest-api-specs/node_modules/oav/lib/validate.js:73:20)

    at Context.<anonymous> (/home/travis/build/Azure/azure-rest-api-specs/test/semantic.js:19:44)

    at callFn (/home/travis/build/Azure/azure-rest-api-specs/node_modules/mocha/lib/runnable.js:378:21)

    at Test.Runnable.run (/home/travis/build/Azure/azure-rest-api-specs/node_modules/mocha/lib/runnable.js:370:7)

    at Runner.runTest (/home/travis/build/Azure/azure-rest-api-specs/node_modules/mocha/lib/runner.js:442:10)

    at /home/travis/build/Azure/azure-rest-api-specs/node_modules/mocha/lib/runner.js:560:12

    at next (/home/travis/build/Azure/azure-rest-api-specs/node_modules/mocha/lib/runner.js:356:14)

    at /home/travis/build/Azure/azure-rest-api-specs/node_modules/mocha/lib/runner.js:366:7

    at next (/home/travis/build/Azure/azure-rest-api-specs/node_modules/mocha/lib/runner.js:290:14)

    at Immediate.<anonymous> (/home/travis/build/Azure/azure-rest-api-specs/node_modules/mocha/lib/runner.js:334:5)

    at runCallback (timers.js:794:20)

    at tryOnImmediate (timers.js:752:5)

    at processImmediate [as _immediateCallback] (timers.js:729:5) ] }


  0 passing (38ms)

  1 failing


  1) Azure swagger semantic validation:

       specification/search/data-plane/Microsoft.Azure.Search.Service/preview/2016-09-01-preview/searchservice.json should be semantically valid.:

     Unable to read the content or execute "JSON.parse()" on the content of file "specification/search/data-plane/Microsoft.Azure.Search.Service/preview/2016-09-01-preview/searchservice.json". The error is:

SyntaxError: Unexpected token } in JSON at position 140440

Please ensure that autorest . --azure-validator=true runs cleanly on your machine.

brjohnstmsft

One more x-nullable case to look at

brjohnstmsft · 2018-03-30T00:10:40Z

...arch/data-plane/Microsoft.Azure.Search.Service/preview/2016-09-01-preview/searchservice.json

+        "maxFieldsPerIndex": {
+          "type": "integer",
+          "format": "int32",
+          "x-nullable": true,


If all the limits could be null, then what about maxIndexerRunTime? Shouldn't it be x-nullable too for consistency?

natinimni · 2018-03-30T02:30:27Z

Thanks @brjohnstmsft ! maxIndexerRunTime should indeed be nullable :)

brjohnstmsft · 2018-04-02T19:09:09Z

@sergey-shandar Are you taking over the review from @johanste? If so, can you please review as soon as possible, as this is blocking our next release? This release includes a fix for a bug that is affecting our customers, so time is of the essence. Thanks!

sergey-shandar · 2018-04-03T18:57:12Z

@olydis are you aware about this Autorest issue?

sergey-shandar · 2018-04-03T18:59:04Z

...re.Search.Service/preview/2016-09-01-preview/examples/SearchServiceGetServiceStatistics.json

@@ -0,0 +1,43 @@
+{
+    "parameters": {
+        "api-version": "2016-09-01-Preview"


@natinimni could you provide also searchServiceName parameter?

Hi @sergey-shandar, similarly to all other Azure Search APIs, 'searchServiceName' is taken from the URL path (not a query parameters)...

@sergey-shandar Please note that this is a data plane API, not an RP.

Slight correction to what @natinimni said -- The search service name is in the host name, not the URL path (each tenant in Azure Search has their own unique endpoint). I'm not aware of any way to specify such parameters in the example files.

@natinimni @brjohnstmsft I understand that this parameter is translated to a host name but it's still a parameter and it has to be in examples. AFAIK, there are no exceptions by how the parameter is represented on wire.

For example

"parameters": { "api-version": "2016-09-01-Preview", "searchServiceName": "somevalue" },

Thanks @sergey-shandar - I've added searchServiceName to all of out API examples, including this one.

brjohnstmsft · 2018-04-03T22:29:48Z

@sergey-shandar What AutoRest issue are you referring to?

olydis

signoff on the directive

…xample file. Since this may fail some server validation, only adding it here for now and will add it to the rest of the examples if it works

Azure Search: Adding Service Statistics API

f156bd9

azuresdkci assigned sergey-shandar Mar 29, 2018

azuresdkci requested a review from sergey-shandar March 29, 2018 18:00

brjohnstmsft suggested changes Mar 29, 2018

View reviewed changes

natinimni added 2 commits March 29, 2018 14:40

Addressing PR comments

c05ada6

Removing extra ',' char from spec file

6ad0570

brjohnstmsft suggested changes Mar 30, 2018

View reviewed changes

making maxIndexerRunTime nullable

d8d72d8

brjohnstmsft approved these changes Mar 30, 2018

View reviewed changes

Changing ResourceCounter.usage to not be nullable

3111b3c

brjohnstmsft approved these changes Apr 2, 2018

View reviewed changes

sergey-shandar added WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Apr 3, 2018

sergey-shandar requested a review from olydis April 3, 2018 18:56

sergey-shandar suggested changes Apr 3, 2018

View reviewed changes

olydis approved these changes Apr 4, 2018

View reviewed changes

natinimni added 2 commits April 4, 2018 10:49

Adding searchServiceName as a parameter in the getServiceStatistics e…

79c34b8

…xample file. Since this may fail some server validation, only adding it here for now and will add it to the rest of the examples if it works

Adding a 'searchServiceName' parameter to all example files

8bb30f1

brjohnstmsft approved these changes Apr 4, 2018

View reviewed changes

sergey-shandar approved these changes Apr 4, 2018

View reviewed changes

sergey-shandar merged commit dbace6a into Azure:master Apr 4, 2018

natinimni mentioned this pull request Apr 5, 2018

[Azure Search] Add service stats API Azure/azure-sdk-for-net#4191

Merged

9 tasks

natinimni deleted the azure-search-add-service-stats-api branch April 5, 2018 20:52

brjohnstmsft mentioned this pull request Apr 10, 2018

[Azure Search] Merge latest preview changes to psSdkJson6 Azure/azure-sdk-for-net#4204

Merged

9 tasks

Conversation

natinimni commented Mar 29, 2018 • edited 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 29, 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!

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

natinimni commented Mar 29, 2018

Uh oh!

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

natinimni Apr 2, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natinimni Apr 3, 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!

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!

brjohnstmsft commented Mar 29, 2018

Uh oh!

brjohnstmsft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natinimni commented Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brjohnstmsft commented Apr 2, 2018

Uh oh!

sergey-shandar commented Apr 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natinimni Apr 3, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

natinimni commented Mar 29, 2018 •

edited

Loading

AutorestCI commented Mar 29, 2018 •

edited

Loading

AutorestCI commented Mar 29, 2018 •

edited

Loading

AutorestCI commented Mar 29, 2018 •

edited

Loading

AutorestCI commented Mar 29, 2018 •

edited

Loading

natinimni Apr 2, 2018 •

edited

Loading

natinimni Apr 3, 2018 •

edited

Loading

natinimni commented Mar 30, 2018 •

edited

Loading

natinimni Apr 3, 2018 •

edited

Loading

sergey-shandar Apr 4, 2018 •

edited

Loading

olydis left a comment •

edited

Loading