Skip to content
This repository was archived by the owner on May 5, 2023. It is now read-only.

Adding App Insights data plane client#3021

Closed
alexeldeib wants to merge 8 commits intoAzure:masterfrom
alexeldeib:appinsights
Closed

Adding App Insights data plane client#3021
alexeldeib wants to merge 8 commits intoAzure:masterfrom
alexeldeib:appinsights

Conversation

@alexeldeib
Copy link
Contributor

This PR adds a new client with a Query operation group similar to #3008, as well as metrics and events operation groups.

Testing is WIP, but figured I would open this up for feedback. Looking forward to it 😊

@ghost ghost added the in progress label Jun 17, 2018
@azuresdkci
Copy link

Can one of the admins verify this patch?

@amarzavery
Copy link
Contributor

There is a separate repo https://github.com/Microsoft/ApplicationInsights-node.js/ for application insights. How is this different than that? Why would one need this over the other one? Don't you think this will create lot of confusion for our customers?

package.json Outdated
"Krishnan, Balaji <[email protected]>"
],
"version": "2.3.1-preview",
"version": "2.3.2-preview",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We control shipping the rollup version. We don't plan to ship the roll up version anytime soon. Please revert the change in this package.json file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do -- previously was told to bump this for Log Analytics so I did the same here. I'll revert.

```javascript
const msRest = require("ms-rest");
const ApplicationInsightsDataClient = require("azure-applicationinsights");
const token = "<access_token>";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From where would one get the access token. Please provide that info. Aren't customers suppose to use the standard msRestAzure.loginWithServicePrincipal(..) method for authentication?

@alexeldeib
Copy link
Contributor Author

Totally agree about the confusion, I wasn't aware of any data plane SDKs hosted in separate repositories. For context we're in the realm of monitoring so I'd broadly categorize the functionality into three groups: management, data plane access, data plane ingestion (telemetry client).

The SDK you linked is for application instrumentation and telemetry ingestion into the data plane. I'm not sure why it's a standalone repository. I would have to sync with the owners in that area. Notably, it looks like this SDK was not generated from Swagger? I don't see a specification for anywhere, and the structure looks quite different. I do see that other ingestion telemetry clients hosted in their own repositories, e.g.

The management functionality for App Insights is already in azure-sdk-for-node: https://github.com/Azure/azure-sdk-for-node/tree/master/lib/services/applicationinsights

This PR adds data access (egress) via endpoints to query logs, events, and metric values. l'll continue to add examples and tests.

I'm not really sure what a clean resolution looks like here. The history of the ingestion SDKs is somewhat beyond the context I have available. If you have any thoughts on a preferred way to resolve this, I'd be happy to hear them.

@amarzavery
Copy link
Contributor

amarzavery commented Jun 19, 2018

I would recommend to put all the information you provided in the above comment in the readme.md of azure-applicationinsights and azure-arm-applicationinsights.

This info should be at the top. People search for packages on npm. The info in readme can be seen over there. This will make the purpose of each of the libraries very clear.

Also mention about the ingress library and a link to it's npm package.

Put keywords like management, egress, etc in package.json.

@alexeldeib
Copy link
Contributor Author

alexeldeib commented Jun 19, 2018

Sounds good, I can add clarification to the readmes.

I've been adding some basics tests and bumped into an issue. We have one operation under the events group which returns XML response for Odata metadata. The Swagger for the operation has this field as

"produces": [
    "application/xml; charset=UTF-8"
]

The client is trying to deserialize this as JSON, which fails. Is this scenario currently supported, or is there anything obvious I've missed in modeling an XML response?

@RikkiGibson
Copy link
Member

XML is a new feature in our WIP (largely stabilized) isomorphic TypeScript toolchain--if you want to release a client library that requires XML [de]serialization, you'll have to get on board over there. I believe Batch among others have been doing this in their development phase already.

See https://github.com/azure/autorest.typescript and https://github.com/azure/ms-rest-js

@alexeldeib
Copy link
Contributor Author

@RikkiGibson I played around with this. The transform to string in swagger doesn't work because the client still parses the response as JSON.

If you have any guidance on writing extensions to the generated code, I can add a couple lines to deserialize it. Otherwise, I'm fine leaving as-is and I can make a note in the readme to use "raw = true" and that XML deserialization needs to be done manually. Down the road, we could migrate to the typescript tooling and add that support.

@RikkiGibson
Copy link
Member

Thanks for having a look. I wouldn't bend over backwards to add features to a nodejs SDK at this point as we're looking forward to just solving the problems in the isomorphic toolchain.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants