HTTP dependency request tracking #129

jasongin · 2016-10-28T22:41:29Z

This change adds automatic collection of dependency HTTP request data.

Please check that all the RemoteDependencyData fields are filled in correctly in the ClientRequestParser.getDependencyData() method. I didn't know where to find good documentation for those fields so in some cases I had to infer the correct usage by looking at other code.

The GitHub web diff view unfortunately doesn't compare some files that were renamed and have only minor changes:

Requests.ts -> ServerRequests.ts
RequestDataHelper.ts -> ServerRequestParser.ts
RequestDataHelper.tests.ts -> ServerRequestParser.ts

Unit tests all pass. I also created a small test Node.js service to verify that both request and dependency telemetry is successfully uploaded. The dependencies do show up in the Application Map!

Add automatic collection of dependency HTTP request data.

OsvaldoRosado · 2016-10-28T23:22:03Z

Library/Client.ts

    }

-    public trackRequest(request: http.ServerRequest, response: http.ServerResponse, properties?: { [key: string]: string; }) {
-        RequestTracking.trackRequest(this, request, response, properties);
+    public trackServerRequest(request: http.ServerRequest, response: http.ServerResponse, properties?: { [key: string]: string; }) {


Is this a breaking change? Would it be better to continue calling this trackRequest and merging the new trackClientRequest with trackDependency for compatibility with previous versions and consistency with AI terms for what these are?

Yes, it's a breaking change. I suppose renaming this method is not that important.

However merging trackClientRequest with trackDependency doesn't make sense to me: the parameters are completely different, even though they end up doing something similar.

We should support both methods trackRequest and trackDependency. See API Summary

Yes, but what about the trackClientRequest method I have added here? Without that, it would be difficult for an application to manually track HTTP dependency requests, because it takes a few dozen non-obvious lines of code to convert a http.ClientRequest into appropriate parameters for trackDependency. (Perhaps it could be renamed trackDependencyRequest instead of trackClientRequest.)

trackDependencyRequest sounds good.

I corrected the naming of Client methods. Now there is:

trackRequest - unchanged

trackDependency - added optional target parameter at the end

trackDependencyRequest - new, alternative to trackDependency that is appropriate to use if you have a http.ClientRequest object that you want to track manually (when autocollection is disabled).

KamilSzostak · 2016-10-28T23:28:36Z

Thanks for sending a pull request! This is a really good addition to the Node.js SDK.

I agree with @OsvaldoRosado, we should keep the naming consistent with other AppInsights SDKs. This is "Request" (for all requests that node.js is receiving) and "Dependency" (for all requests the node.js is sending).

jasongin · 2016-10-28T23:35:19Z

Library/Client.ts

    }

    public trackDependency(
        name: string,
        commandName: string,
+        target: string,


The addition of the target parameter here is a breaking change. To avoid that it will need to be moved to an optional parameter at the end, although I think the intention of the new contract is actually that it should be required.

I don't think our bond currently asks for this field to be required. It might not be so bad to move this to the end and mark optional. Anyone have strong thoughts on that?

EDIT: Is it possible to fill this by default if its left unfilled? Our bond marks commandName as deprecated in favor of data. If this is filled like data it would include "http urls with all query parameters"

I don't see what "data" field you are referring to. RemoteDependencyData doesn't have a "data" field.

The target could be auto-filled by parsing the hostname out of the supplied commandName argument if it is a valid URI, or the name argument if it is a HTTP method + URI.

Sorry, the data field is one we're not using in this SDK yet, but is what commandName is migrating to eventually, as we deprecated it in the schema. I was just noting, as you did, that we can get the hostname out of commandName, based off of what the schema said the field should contain.

With this in mind, we should move target to the end and mark optional. Like @KamilSzostak mentions below, this avoids breaking existing users.

OK, I moved target to the end. If not supplied it will attempt to auto-fill by parsing the commandName.

OsvaldoRosado · 2016-10-28T23:43:12Z

This is a really good addition to the Node.js SDK.

I agree, this is awesome 👍

KamilSzostak · 2016-10-28T23:35:18Z

AutoCollection/ClientRequestParser.ts

+        let urlObject = url.parse(this.url);
+        urlObject.search = undefined;
+        urlObject.hash = undefined;
+        let dependencyName = this.method + " " + url.format(urlObject);


Could you make sure that this.method is upper-cased? It should be consistent with the JavaScript SDK - see

KamilSzostak · 2016-10-28T23:48:27Z

AutoCollection/ClientRequests.ts

+            const request: http.ClientRequest = originalRequest.call(
+                    originalRequest, options, ...requestArgs);
+            if (request) {
+                AutoCollectClientRequests.trackRequest(this._client, options, request);


Would it be possible to reuse trackDependency method? Ideally, we want to have only one function to build a RemoteDependencyData object.

Possible, but I think that would make the code less consistent overall.

This dependency request code is following the same pattern as the already existing server request code. That code does not call the Client.trackRequest method either: the helper constructs a RequestData object directly instead.

OK, let's keep it as it is.

KamilSzostak · 2016-10-29T00:13:16Z

Library/Client.ts

    }

    public trackDependency(
        name: string,
        commandName: string,
+        target: string,


Can you add target as the last parameter and mark it as optional? This way we won't break existing users.

Addressing review feedback and fixing bugs found in integration testing.

OsvaldoRosado · 2016-11-01T21:22:55Z

Library/Client.ts

+        target: string = null) {
+
+        if (!target && commandName) {
+            target = url.parse(commandName).host;


Does url.parse always return an object?

Yes. AFAIK it never throws unless you pass it a non-string argument. An incomplete URI is just parsed as a relative path, and any invalid characters get encoded.

KamilSzostak · 2016-11-01T21:33:37Z

Tests/Library/Client.tests.ts

@@ -306,7 +315,7 @@ describe("Library/Client", () => {
                assert.ok(trackStub.calledOnce);
                var args = trackStub.args;
                assert.equal(args[0][0].baseType, "Microsoft.ApplicationInsights.RequestData");
-                assert.equal(args[0][0].baseData.responseCode, 200);


Is responseCode gone?

It's not gone, but it is not 200 anymore. This test is simulating a failed request, so the responseCode never should have been 200. My refactor of the RequestDataHelper/RequestParser class fixed this bug.

KamilSzostak · 2016-11-01T21:41:12Z

I added a small comment about responseCode in trackRequest() unit test. Everything else looks fine.

jasongin · 2016-11-02T17:27:01Z

Should this include a minor version bump in package.json, since it is a significant new feature?

KamilSzostak · 2016-11-02T18:28:18Z

No, no need to change the version number. I usually change it when I merge from a develop to master branch.

jasongin · 2016-11-02T20:27:44Z

Closing this, because I have resubmitted as a new PR #130, to target the develop branch.

HTTP dependency request tracking

bf1bf9b

Add automatic collection of dependency HTTP request data.

OsvaldoRosado reviewed Oct 28, 2016

View reviewed changes

jasongin commented Oct 28, 2016

View reviewed changes

KamilSzostak reviewed Nov 1, 2016

View reviewed changes

HTTP dependencies - fixes

0318948

Addressing review feedback and fixing bugs found in integration testing.

OsvaldoRosado reviewed Nov 1, 2016

View reviewed changes

KamilSzostak reviewed Nov 1, 2016

View reviewed changes

jasongin mentioned this pull request Nov 2, 2016

HTTP dependency request tracking #130

Merged

jasongin closed this Nov 2, 2016

OsvaldoRosado mentioned this pull request Apr 10, 2017

ClientRequests -> OutgoingHttpDependencies #226

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTP dependency request tracking #129

HTTP dependency request tracking #129

jasongin commented Oct 28, 2016 •

edited

Loading

OsvaldoRosado Oct 28, 2016

jasongin Oct 28, 2016

KamilSzostak Nov 1, 2016

jasongin Nov 1, 2016

KamilSzostak Nov 1, 2016

jasongin Nov 1, 2016

KamilSzostak commented Oct 28, 2016

jasongin Oct 28, 2016

OsvaldoRosado Oct 28, 2016 •

edited

Loading

jasongin Nov 1, 2016

OsvaldoRosado Nov 1, 2016

jasongin Nov 1, 2016

OsvaldoRosado commented Oct 28, 2016

KamilSzostak Oct 28, 2016

jasongin Nov 1, 2016

KamilSzostak Oct 28, 2016

jasongin Nov 1, 2016

KamilSzostak Nov 1, 2016

KamilSzostak Oct 29, 2016

jasongin Nov 1, 2016

OsvaldoRosado Nov 1, 2016

jasongin Nov 1, 2016

KamilSzostak Nov 1, 2016

jasongin Nov 1, 2016

KamilSzostak commented Nov 1, 2016

jasongin commented Nov 2, 2016

KamilSzostak commented Nov 2, 2016

jasongin commented Nov 2, 2016

HTTP dependency request tracking #129

HTTP dependency request tracking #129

Conversation

jasongin commented Oct 28, 2016 • edited Loading

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

KamilSzostak commented Oct 28, 2016

Choose a reason for hiding this comment

OsvaldoRosado Oct 28, 2016 • edited Loading

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

OsvaldoRosado commented Oct 28, 2016

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

KamilSzostak commented Nov 1, 2016

jasongin commented Nov 2, 2016

KamilSzostak commented Nov 2, 2016

jasongin commented Nov 2, 2016

jasongin commented Oct 28, 2016 •

edited

Loading

OsvaldoRosado Oct 28, 2016 •

edited

Loading