Add Log Analytics data plane SDK #2557

alexeldeib · 2018-05-10T09:24:23Z

Hi all! I'm introducing a new client to query Log Analytics (the operational insights resource provider in Azure). Tested out the SDK manually and it looks great, had a couple questions though!

I saw the testing using stubbed management clients, is there an equivalent for data plane clients? or can I implement some testing using that framework and custom endpoints? I looked to KeyVault often as an example of a service slightly parallel our own -- custom endpoint, auth resource, etc, but they don't have tests? (after thinking about this some more, I misread the original code I think -- looks like I just need to new up a client).
Due to swagger limitations we have one field of our response type that's modeled as a string (it's the type of the row value in the tables) but on the wire can actually be pretty much any primitive type: string, int, null, float/double, some datetime formats. In C# we are somewhat forced to deal with the strings but we have type information returned by the API to type them appropriately. I haven't tested this bit too closely, but will the python client respect the wire format, or actually use the attribute mappings to cast things to string?

rest-api-specs spec

azuresdkci · 2018-05-10T09:24:24Z

Can one of the admins verify this patch?

codecov-io · 2018-05-10T09:42:28Z

Codecov Report

Merging #2557 into master will increase coverage by 0.03%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #2557      +/-   ##
==========================================
+ Coverage   56.71%   56.74%   +0.03%     
==========================================
  Files        7647     7666      +19     
  Lines      158352   158544     +192     
==========================================
+ Hits        89803    89963     +160     
- Misses      68549    68581      +32

Impacted Files	Coverage Δ
...re-loganalytics/azure/loganalytics/models/table.py	`100% <100%> (ø)`
...ganalytics/azure/loganalytics/models/query_body.py	`100% <100%> (ø)`
azure-loganalytics/azure/loganalytics/version.py	`100% <100%> (ø)`
...ics/azure/loganalytics/models/query_results_py3.py	`100% <100%> (ø)`
azure-loganalytics/azure/loganalytics/__init__.py	`100% <100%> (ø)`
...oganalytics/azure/loganalytics/models/table_py3.py	`100% <100%> (ø)`
azure-loganalytics/azure/__init__.py	`100% <100%> (ø)`
...e-loganalytics/azure/loganalytics/models/column.py	`100% <100%> (ø)`
...ganalytics/azure/loganalytics/models/column_py3.py	`100% <100%> (ø)`
...loganalytics/azure/loganalytics/models/__init__.py	`100% <100%> (ø)`
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa8ae2b...1c35da8. Read the comment docs.

lmazuel · 2018-05-16T23:52:19Z

Hi @alexeldeib !
Sorry for the late answer, I was in PyCon improving my Python last week :)

Most important here is that Python has no history to follow, so the mgmt package was release using the correct loganalytics branding.
https://pypi.org/project/azure-mgmt-loganalytics/#description

This has to be the same here, but overriding package name and title, like I did here:
https://github.com/Azure/azure-rest-api-specs/tree/master/specification/operationalinsights/resource-manager#python

Also, it seems to be not ARM, so the correct dependency will be "msrest" and not "msrestazure":
https://github.com/Azure/azure-sdk-for-python/blob/master/azure-servicefabric/setup.py#L80

To answer your questions now:

The test framework is not tight to ARM, ARM is just a default. Documentation: https://github.com/Azure/azure-sdk-for-python/wiki/Contributing-to-the-tests
Python is awesome, usually we can do anything we want :). I just don't have enough information to answer, could you show me some JSON example with the corresponding Swagger line?

Thanks!

lmazuel · 2018-05-16T23:54:20Z

Note, the workspace_id fix as to be done in the Swagger, change this line value to "method":
https://github.com/Azure/azure-rest-api-specs/blob/master/specification/operationalinsights/data-plane/Microsoft.OperationalInsights/stable/v1/OperationalInsights.json#L131

alexeldeib · 2018-05-21T08:55:52Z

PyCon sounds exciting! Hope it was fun 👍

I missed the management packages -- I'll change the naming to match. I'll also fix the dependency in setup.py.

On the point about workspace ID, coincidentally I have a PR out with that change 😄 I also updated the readme there for generation which (of course) produced another PR.

To the last bit about the string parameter: this is the field in the swagger. I saw models all have code like this:

_attribute_map = {
        'name': {'key': 'name', 'type': 'str'},
        'columns': {'key': 'columns', 'type': '[Column]'},
        'rows': {'key': 'rows', 'type': '[[str]]'},
    }

I had a chance to test this out and I see see that the python client does respect the string types in the swagger, converting the JSON type, e.g.:

[
  u'OpsManager', 
  u'2018-04-08T12:27:10.86Z',
  u'00000000-0000-0000-0000-000000000001',
  u'40.83.215.18', 
  u'Direct Agent', 
  u'Windows', 
  u'', 
  u'10', 
  u'0', 
  u'8.0.11081.0', 
  u'Direct', 
  'False', 
  '-121.83',
  '37.32',
  u'United States', 
  u'microsoft.compute', 
  u'virtualmachines', 
  u'Azure', 
  u'"security", "serviceMap", "securityCenterFree"',
  u'Heartbeat'
]

For context, here's the JSON returned from the HTTP call (note the boolean and raw number values returned, converted to strings):

[
    "OpsManager",
    "2018-04-22T08:40:09.77Z",
    "00000000-0000-0000-0000-000000000001",
    "40.83.215.18",
    "Direct Agent",
    "Windows",
    "",
    "10",
    "0",
    "8.0.11081.0",
    "Direct",
    false,
    -121.83,              # HERE 
    37.32,
    "United States",
    "microsoft.compute",
    "Azure",
    "\"security\", \"serviceMap\", \"securityCenterFree\"",
    "Heartbeat"
]

Answering my own question -- the client will respect whatever is in the swagger. My follow up, then: we have the information for column types. Could I do some custom modeling to convert/cast the underlying values so they look right from the service perspective? For example, a value returned as a number in JSON will come back as a string but I would convert it back using the column.type field to determine type.

We are returning rows from DB query, but due to restriction in swagger/autorest we can't model the value as multiple primitive types (yet!). So we ended up with string. I'm trying to work around that at the SDK level to provide a better experience. Curious what you think.

lmazuel · 2018-05-25T18:13:30Z

@alexeldeib Yes, we can do custom serialization/deserialization. But in this case, it seems I just have to not try to hard and expose what was on the JSON right? Could you confirm this is all basic types? For instance, there is not date in ISO8601 string your expect to be exposed as datetime object?

If this is just exposing bool/str/int/float, I can probably add an internal option in my runtime to say "Don't try serialize/deserialize". Example:

_attribute_map = {
        'name': {'key': 'name', 'type': 'str'},
        'columns': {'key': 'columns', 'type': '[Column]'},
        'rows': {'key': 'rows', 'type': '[[str]]', 'transform': False},
    }

Thoughts?

alexeldeib · 2018-05-25T19:21:01Z

Your proposal is the ideal. We really want to expose what the API brings back, because it "knows better" than the swagger. There are some ISO8601 format strings, but we are modeling these as strings in swagger anyway (due to format limitations), so this shouldn't present a problem python-side.

BTW -- I'm waiting to move forward here until the swagger-side cleanup from the above PR is complete. It includes the workspace_id => method parameter change, as well as some more general housekeeping.

lmazuel · 2018-05-25T23:45:33Z

@alexeldeib Swagger (at least Autorest, maybe there is subtle difference between Autorest and Swagger here) supports ISO8601, and we have automatic transformer for that.

You should need to use format "date-time" and on the SDK side you receive a date object ( datetime for Python). Example:
https://github.com/Azure/azure-rest-api-specs/blob/0bef2b99ad6d52781e3c308bc6b420f6eef59ef7/specification/apimanagement/resource-manager/Microsoft.ApiManagement/stable/2016-10-10/apimnetworkstatus.json#L105

What I saw sometimes, in that C# serializes by default 7 level microseconds precision, which is not handled by all languages. It's why Microsoft OneApi guideline asks explicitly for 3 level precision and not more, to accommodate all languages. There is some Swagger that are designed as string because of that, because it's not a valid ISO8601 string for Microsoft OneApi. If your server does that, yes you have to use string :/

alexeldeib · 2018-05-26T00:05:50Z

Side bar -- for the results, we're actually modeled just as string in swagger. This was the swagger/autorest limitation -- giving this a variety of type possibilities. So we landed on just making all the row results string in swagger. The comments about timestamps apply to parameters, but not results.

So, what we found with C# was actually that date time worked fine, but that we didn't have good support for ISO8601 intervals, which we use (e.g. query timespan). Do you happen to know if that would work well in Python? A cursory search tells me the standard datetime objects can do durations but not intervals. I haven't played around this with the SDK, which would answer my question...but I figured you would know best 😄

If it ends up that the intervals/durations work well, I'll add a transform in the swagger to include format: duration/format: interval (or the appropriate keywords, per your guidance) to the swaggers for client generation. I think for ISO8601 interval in particular we didn't find a good "format" keyword to feed autorest to get the desired result.

lmazuel · 2018-05-29T16:18:24Z

@alexeldeib You mean intervals like 2007-03-01T13:00:00Z/2008-05-11T15:30:00Z? Yes, Python does not support it right now.
Same here, makes sense to me to support this format natively in the Python runtime of Autorest since it's a standard. I won't be able to declare it in the Swagger, but if the runtime supports it we can can "hack" with changing the type like I suggest for transform.

alexeldeib · 2018-06-13T19:50:22Z

@lmazuel The corresponding updates were merged in the specification repo, I regenerated and added a basic test case for usage. Functionally, from my end this is good to go. Let me know if you still see anything that wasn't covered.

One thing I noticed with the autorest configuration -- there is some weird behavior here.

If I try to override the title, namespace, and package name for python, the namespace and package name work. The title does not override. This looks like what you did with the resource-manager package, but it didn't work for me?
If I leave the global section empty, and only populate the per-language sections for Python, I get a client named "Azure Log Analytics"
If I change the global title, Python uses it and generates "LogAnalyticsDataPlaneClient" as desired

Not sure what autorest is doing here, but it doesn't seem to respect the "title" override. For the time being, I manually generated the SDK with the global title "LogAnalyticsDataPlane" rather than "OperationalInsightsDataPlane". This is the diff of the readme I used against master. This probably means the AutoPR's will be messed up, but I assume the autorest overrides won't work in other languages either. That may take some deeper investigation to resolve.

lmazuel · 2018-06-13T22:10:53Z

You're 100% right @alexeldeib we discovered that with @fearthecowboy recently. There is no way to make the override to work inside the Python section, it has to be moved into the global section.
Plan B would be to override the bot configuration for auto-PR, but this means manual generation won't have it.
I will try to work with @fearthecowboy to find the correct Readme asap.
Thanks a lot for your work here!

alexeldeib · 2018-06-13T23:07:31Z

override-client-name: <name> seems to do the job. The title override seems to work but description still doesn't. I removed the description from the readme which forces Autorest to pull it out of the swagger itself -- that's accurate, at least 😄

Here's the readme diff.

The generated code is identical except for the description [diff]

So I see three options:

Keep the global description as "Operational Insights" and the python description as "Log Analytics". When autorest overrides for description work, you'll get an auto PR that fixes the single-line diff.
Remove global and Python level descriptions, allowing Autorest to pull from the swagger. Only downside here is the text now says "API", but circumvents the problem entirely.
Wait until this is fixed in autorest to merge at all.

Personally, I'd prefer to avoid option 3 -- it doesn't make sense to block on that single line description. I've gone with the second option for right now. If you think that's okay, I'll merge the corresponding readme diff into the spec repo. Or if you prefer the first option, I can make the change here and merge the corresponding readme change as well.

lmazuel · 2018-06-13T23:48:14Z

I like your diff, just want @fearthecowboy feedback on using override-client-name since I didn't this existed... Just want to be sure it's supported correctly (Autorest has some legacy and I want to avoid legacy stuff if possible)

alexeldeib · 2018-06-13T23:52:44Z

Sounds good, no problem to wait. I didn't know about this either 😄 I found it when I went to generate NodeJS library, someone did the same for Log Analytics/Operational Insights already on the resource manager side: https://github.com/Azure/azure-rest-api-specs/blob/master/specification/operationalinsights/resource-manager/readme.nodejs.md

alexeldeib · 2018-06-19T18:59:05Z

@fearthecowboy any comment on this?

fearthecowboy · 2018-06-19T19:45:37Z

I have no idea where override-client-name came from -- that looks like a last minute hack put in before we split out the generators.

it does look like it's in the different generators, so you should be ok.

alexeldeib · 2018-06-19T19:48:30Z

Is there a preferred way to do the same transform otherwise? Definitely happy to avoid using a hack, if there is another approach.

alexeldeib · 2018-06-28T19:01:12Z

@lmazuel any thoughts on this? I looked at this in the generators and i'm fine with using it.

lmazuel · 2018-06-28T22:42:00Z

@alexeldeib I'm a little confused about the relation between this PR and #2767? Should not one of them being closed?

alexeldeib · 2018-06-29T16:46:53Z

No, these are distinct. I'll follow up offline with some additional detail.

alexeldeib · 2018-07-03T22:40:43Z

@lmazuel Let me know if you see any other issues. Both this and #2767 work smoothly now 👍

alexeldeib · 2018-07-11T17:33:01Z

Bump? 😊 @lmazuel

alexeldeib · 2018-07-11T22:35:06Z

Looks good to me! Thanks @lmazuel

lmazuel · 2018-07-11T23:32:12Z

Great! Let's release this guy :)

alexeldeib added 2 commits May 10, 2018 01:53

Add Operational Insights query SDK

4c95bc9

pcakage service mapping

5e60481

alexeldeib requested a review from lmazuel as a code owner May 10, 2018 09:24

make workspace ID method parameter for client generation

df87602

This was referenced May 29, 2018

Support for ISO8601 intervals Azure/msrest-for-python#103

Open

Support returning a field as "raw" Azure/msrest-for-python#104

Closed

alexeldeib and others added 4 commits June 12, 2018 10:31

Regenerate from new spec

f132be7

update query operations

8339b36

Adding test case and recording

e01bf40

update naming in package mapping

25a34c0

Fixed description

d33cc0b

alexeldeib mentioned this pull request Jun 17, 2018

Adding App Insights data plane client #2767

Merged

alexeldeib added 3 commits July 2, 2018 14:26

regen with directive to serialize as object

fa278ff

add tests to validate types

4f999cf

regen using autorest@latest (i think)

68e9815

alexeldeib and others added 5 commits July 11, 2018 10:50

add codeowner

910bcf0

regen using 3.0

0603d8c

fix test issue

7d5a42c

Merge remote-tracking branch 'origin/master' into loganalytics

94306fd

DT LogAnalytics packaging

1c35da8

lmazuel changed the title ~~Add Operational Insights data plane SDK~~ Add Log Analytics data plane SDK Jul 11, 2018

lmazuel merged commit 143c5e3 into Azure:master Jul 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Log Analytics data plane SDK #2557

Add Log Analytics data plane SDK #2557

alexeldeib commented May 10, 2018 •

edited

Loading

azuresdkci commented May 10, 2018

codecov-io commented May 10, 2018 •

edited

Loading

lmazuel commented May 16, 2018

lmazuel commented May 16, 2018

alexeldeib commented May 21, 2018

lmazuel commented May 25, 2018

alexeldeib commented May 25, 2018

lmazuel commented May 25, 2018 •

edited

Loading

alexeldeib commented May 26, 2018

lmazuel commented May 29, 2018

alexeldeib commented Jun 13, 2018

lmazuel commented Jun 13, 2018

alexeldeib commented Jun 13, 2018

lmazuel commented Jun 13, 2018

alexeldeib commented Jun 13, 2018 •

edited

Loading

alexeldeib commented Jun 19, 2018

fearthecowboy commented Jun 19, 2018

alexeldeib commented Jun 19, 2018

alexeldeib commented Jun 28, 2018

lmazuel commented Jun 28, 2018

alexeldeib commented Jun 29, 2018

alexeldeib commented Jul 3, 2018

alexeldeib commented Jul 11, 2018

alexeldeib commented Jul 11, 2018

lmazuel commented Jul 11, 2018

Add Log Analytics data plane SDK #2557

Add Log Analytics data plane SDK #2557

Conversation

alexeldeib commented May 10, 2018 • edited Loading

azuresdkci commented May 10, 2018

codecov-io commented May 10, 2018 • edited Loading

Codecov Report

lmazuel commented May 16, 2018

lmazuel commented May 16, 2018

alexeldeib commented May 21, 2018

lmazuel commented May 25, 2018

alexeldeib commented May 25, 2018

lmazuel commented May 25, 2018 • edited Loading

alexeldeib commented May 26, 2018

lmazuel commented May 29, 2018

alexeldeib commented Jun 13, 2018

lmazuel commented Jun 13, 2018

alexeldeib commented Jun 13, 2018

lmazuel commented Jun 13, 2018

alexeldeib commented Jun 13, 2018 • edited Loading

alexeldeib commented Jun 19, 2018

fearthecowboy commented Jun 19, 2018

alexeldeib commented Jun 19, 2018

alexeldeib commented Jun 28, 2018

lmazuel commented Jun 28, 2018

alexeldeib commented Jun 29, 2018

alexeldeib commented Jul 3, 2018

alexeldeib commented Jul 11, 2018

alexeldeib commented Jul 11, 2018

lmazuel commented Jul 11, 2018

alexeldeib commented May 10, 2018 •

edited

Loading

codecov-io commented May 10, 2018 •

edited

Loading

lmazuel commented May 25, 2018 •

edited

Loading

alexeldeib commented Jun 13, 2018 •

edited

Loading