-
Notifications
You must be signed in to change notification settings - Fork 141
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HTTP dependency request tracking #130
Conversation
Add automatic collection of dependency HTTP request data.
|
remoteDependency.async = true; | ||
remoteDependency.dependencyTypeName = | ||
ContractsModule.Contracts.DependencyKind[ContractsModule.Contracts.DependencyKind.Http]; | ||
remoteDependency.dependencyKind = ContractsModule.Contracts.DependencyKind.Http; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe a new dependency type, something called AI dependency, is being introduced, and that would be used instead of HTTP for communications between AI components.
Check with Nizar on the details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'll include that in the follow-up change that adds component correlation. But for now this code doesn't know when a dependency is an AI component or not.
// response listeners | ||
if (response && response.once) { | ||
response.once("finish", () => { | ||
AutoCollectServerRequests.endRequest(client, requestParser, response, null, properties, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed that in both the Server and Client auto collectors, it's currently not parsing or generating a hashed ikey and on the HTTP headers. This will be needed for stitching different AI components in the map. Perhaps that's the next step?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that will be in a separate PR. I'm testing that now.
I'm resubmitting this as a new PR to target the develop branch instead of the master branch. See review comments on the original PR at #129. There are no changes here compared to that one, aside from rebasing on top of develop.