Adding App Insights data plane client #2767

alexeldeib · 2018-06-17T22:42:41Z

This PR has a very similar Query operation group to #2557, which also means that Azure/msrest-for-python#103 (ISO8601 intervals) and Azure/msrest-for-python#104 apply (raw field without transformation).

Azure/azure-rest-api-specs#3257 adds the configuration for auto PRs.

I'm still adding tests, but wanted to begin the review process since the client works nicely 😄

azuresdkci · 2018-06-17T22:42:42Z

Can one of the admins verify this patch?

azuresdkci · 2018-06-17T22:42:43Z

Can one of the admins verify this patch?

codecov-io · 2018-06-17T22:59:14Z

Codecov Report

Merging #2767 into master will increase coverage by 0.19%.
The diff coverage is 77.61%.

@@            Coverage Diff             @@
##           master    #2767      +/-   ##
==========================================
+ Coverage   56.71%   56.91%   +0.19%     
==========================================
  Files        7647     7772     +125     
  Lines      158352   159826    +1474     
==========================================
+ Hits        89803    90958    +1155     
- Misses      68549    68868     +319

Impacted Files	Coverage Δ
...applicationinsights/models/events_user_info_py3.py	`100% <100%> (ø)`
.../models/metrics_post_body_schema_parameters_py3.py	`100% <100%> (ø)`
azure-applicationinsights/azure/__init__.py	`100% <100%> (ø)`
...licationinsights/models/metrics_result_info_py3.py	`100% <100%> (ø)`
.../applicationinsights/models/metrics_result_info.py	`100% <100%> (ø)`
...re/applicationinsights/models/query_results_py3.py	`100% <100%> (ø)`
...ghts/models/metrics_post_body_schema_parameters.py	`100% <100%> (ø)`
...ure/applicationinsights/models/events_user_info.py	`100% <100%> (ø)`
...licationinsights/models/events_application_info.py	`100% <100%> (ø)`
...e/applicationinsights/models/events_result_data.py	`100% <100%> (ø)`
... and 221 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...0747ab4. Read the comment docs.

alexeldeib · 2018-06-19T17:33:59Z

@lmazuel Does the python SDK support XML deserialization? I see some code for it in ms-rest . I'm seeing however that events_get_odata_metadata (an operation which returns XML), tries to deserialize as JSON.

I also get a log message indicating it's forcing the Accept header to application/json -- figured might be related if the client is expecting JSON.

See test output and relevant swagger lines. I also verified that using 'raw = true', the request is successful and only the deserialization is failing.

alexeldeib · 2018-06-19T17:45:51Z

Better test example, illustrating successful response but failed deserialization

lmazuel · 2018-06-20T20:09:03Z

@alexeldeib , yes starting autorest.python 3.0.53 and msrest 0.5.0, there is XML support! If the content-type is XML.
I see that you return type is "object", you expect Autorest to return the XML tree? I probably don't have any tests for that. @fearthecowboy @RikkiGibson FYI, what would Autorest Typescript or Autorest core think about this?

alexeldeib · 2018-06-20T21:06:51Z

@lmazuel I think that's accurate as to what I would have expected, or if there's a different way to model it that works with the client otherwise I'd be happy to adjust. This also came up with respect to nodejs -- see Azure/azure-sdk-for-node#3021 (comment) Not decided yet on a solution there

RikkiGibson · 2018-06-20T21:13:17Z

I think autorest.typescript will parse the XML and pass it along as a simple object tree, similar to what is returned by JSON.parse(). There will be some special properties to capture things like XML attributes and text content. However I also don't have tests for it so I can't really guarantee what it will do.

lmazuel · 2018-06-20T21:13:27Z

I think it's fine, it's just I didn't consider this scenario. In all transparency, XML is still pretty new and really vague in the Swagger spec, in particular I didn't find anything about free-form object (what you have) and XML. But I guess it makes sense to return the raw XML tree when we see "object". At least on deserialization, I don't think it can be supported on serialization.
You don't need to declare anything in Python, I base everything on HTTP headers. So if your content-type is XML, you'll get XML deserialization. It might already return you a XML tree, I honestly didn't test that scenario.

Edit: on serialization could be done actually, serializing the tree in place without the <xml> header.

lmazuel · 2018-06-22T21:45:28Z

@alexeldeib I released a msrest 0.5.1 that should fixes your XML issue. Please confirm when you have some time :)
Thanks!

alexeldeib · 2018-06-23T20:34:12Z

Looks good 😄 I can see the same test fixed in CI build, thanks @lmazuel !

alexeldeib · 2018-07-12T00:08:28Z

@lmazuel hmm, seems like after using latest generation the XML deserialization no longer works? The raw response is still fine, but the object isn't. Was there anything changed here? I didn't see anything obvious that looked problematic in the code myself

lmazuel · 2018-07-12T19:08:16Z

@alexeldeib I have some deep tests to do on "object" + "additionalProperties", I will double-check this closely, and be sure to add precise test (I did it already, but seems I didn't model your situation close enough :/)

alexeldeib · 2018-07-12T20:08:43Z

@lmazuel I also did some digging into this -- looks like 0.5.1 works, but 0.5.2 breaks. Weird thing is, the parsing works fine when __call__'ed with content-type 'application/xml'.

However, the generated method calls it without a content-type. I'm not sure why this would have worked before, still looking at how to fix. Looking at the diffs i'm not sure how this even broke between 0.5.1 and 0.5.2. The swagger has produces: application/xml, do you have any ideas how to fix the invocation generation here with that additional info?

lmazuel · 2018-07-12T21:07:33Z

@alexeldeib The generation does not use "content_type", because "response" is a HTTP response object, and the content-type is extracted from the headers.
But the generated code is not using the latest generator, and I have XML fixes in 3.0.53 and 3.0.58, could you regenerate this one with "--use=@microsoft.azure/autorest.python@3.0.58" ?

Edit: looking at your recording:

content-type: [application/xml; charset=utf-8]

this means "content_type" is "application/xml" and is not needed

lmazuel · 2018-07-12T21:17:46Z

@alexeldeib ok got it, damn....
This test:
Azure/msrest-for-python@1c19601#diff-3ff87e93ca942963e2ffda1097136bfaR1427

if isinstance(data, ET.Element) and not data.text

does not work if it's <root><child/></root> because root has no text....

I'll improve my if statement and add a test for that.

alexeldeib · 2018-07-12T21:25:35Z

Nice catch, I looked at that if statement but thought it was okay 👍 I will also go ahead and push the 3.0.58 generation.

alexeldeib · 2018-07-12T21:47:37Z

I noticed this broke CI and I fixed the corresponding swagger field in this PR. The runtime is doing the right thing, the backend doesn't actually accept these options.

lmazuel · 2018-07-12T21:59:01Z

Fixed in Azure/msrest-for-python#117

lmazuel · 2018-07-12T22:00:21Z

azure-applicationinsights/azure/applicationinsights/operations/events_operations.py

        # Construct headers
        header_parameters = {}
-        header_parameters['Accept'] = 'application/json;odata=none'
+        header_parameters['Accept'] = 'application/json'


why is that? Is this an Autorest issue? Something weird in the Swagger?

Bug in the swagger on my end, see Azure/azure-rest-api-specs#3409

ok, saw your comment, thanks!

Adding App Insights client

9acf61b

alexeldeib requested a review from lmazuel as a code owner June 17, 2018 22:42

alexeldeib added 7 commits June 17, 2018 16:15

adding initial tests

4c1268e

adding tests

e73cc8c

fix query schema

1d8f50e

remove raw=true

a65311e

assert is not none for example

d47430e

add back disabled tests

f471927

events testing

d0eba88

lmazuel mentioned this pull request Jun 20, 2018

XML and object type Azure/msrest-for-python#111

Merged

remove comment

b753e13

lmazuel mentioned this pull request Jun 28, 2018

Add Log Analytics data plane SDK #2557

Merged

Ace Eldeib and others added 7 commits July 2, 2018 14:08

regen with directives

21b73a2

regen using autorest@latest

4262160

add codeowner

b6e5c8f

regen using 3.0 + add trailing slash to codeowners

f15c7ab

fix keyword args

6f4f9dc

fix tests author missing

bd05509

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

945b82a

AppInsights packaging

b9c88e2

update generation

2d0e09a

fix generation

485be3f

alexeldeib mentioned this pull request Jul 12, 2018

[App Insights] Fix unnecessary "produces" messing up some autorest clients. Azure/azure-rest-api-specs#3409

Merged

10 tasks

add new recordings

9b9286d

lmazuel reviewed Jul 12, 2018

View reviewed changes

Ace Eldeib added 2 commits July 12, 2018 15:23

bump version

fa080b9

rename for pytest unique files(?)

8be3666

test fixes; prettify keyword args

0747ab4

lmazuel merged commit 4009970 into Azure:master Jul 12, 2018

Adding App Insights data plane client #2767

Adding App Insights data plane client #2767

Uh oh!

Conversation

alexeldeib commented Jun 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

azuresdkci commented Jun 17, 2018

Uh oh!

azuresdkci commented Jun 17, 2018

Uh oh!

codecov-io commented Jun 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alexeldeib commented Jun 19, 2018

Uh oh!

alexeldeib commented Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lmazuel commented Jun 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexeldeib commented Jun 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RikkiGibson commented Jun 20, 2018

Uh oh!

lmazuel commented Jun 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lmazuel commented Jun 22, 2018

Uh oh!

alexeldeib commented Jun 23, 2018

Uh oh!

alexeldeib commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lmazuel commented Jul 12, 2018

Uh oh!

alexeldeib commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lmazuel commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lmazuel commented Jul 12, 2018

Uh oh!

alexeldeib commented Jul 12, 2018

Uh oh!

alexeldeib commented Jul 12, 2018

Uh oh!

lmazuel commented Jul 12, 2018

Uh oh!

lmazuel Jul 12, 2018

Choose a reason for hiding this comment

Uh oh!

alexeldeib Jul 12, 2018

Choose a reason for hiding this comment

Uh oh!

lmazuel Jul 12, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alexeldeib commented Jun 17, 2018 •

edited

Loading

codecov-io commented Jun 17, 2018 •

edited

Loading

alexeldeib commented Jun 19, 2018 •

edited

Loading

lmazuel commented Jun 20, 2018 •

edited

Loading

alexeldeib commented Jun 20, 2018 •

edited

Loading

lmazuel commented Jun 20, 2018 •

edited

Loading

alexeldeib commented Jul 12, 2018 •

edited

Loading

alexeldeib commented Jul 12, 2018 •

edited

Loading

lmazuel commented Jul 12, 2018 •

edited

Loading