-
Notifications
You must be signed in to change notification settings - Fork 252
Added AI #35
base: ai
Are you sure you want to change the base?
Added AI #35
Conversation
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.
Would be great to have this changes if possible.
<dependency> | ||
<groupId>com.microsoft.azure</groupId> | ||
<artifactId>applicationinsights-web</artifactId> | ||
<version>[1.0,)</version> |
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.
Update version to 2.0.0-BETA for now and later to [2.0.,) once stable version is out.
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.
Done.
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.
@nicolehaugen79 I still see the dependency version as [1.0,). Did you missed changing to 2.0.0-BETA ?
|
||
@Bean(name = "WebRequestTrackingFilter") | ||
public Filter WebRequestTrackingFilter() { | ||
return new WebRequestTrackingFilter(); |
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.
Well have you tried to instrument Application Insights Agent to collect dependencies in this application? Probably if you would like to collect the calls to documentDB (unfortunately not autocollected now) or other dependencies like HTTP outgoing via Apache HTTP, Redis etc. you would need to put a "name of type string representing application name" as a parameter to WebRequestTrackingFilter() . Feel free to ask for more 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.
As we discussed, there is a bug that is blocking this - I logged an issue to add dependency tracking when the bug is fixed: #36
<Add type="com.microsoft.applicationinsights.web.extensibility.initializers.WebUserTelemetryInitializer"/> | ||
<Add type="com.microsoft.applicationinsights.web.extensibility.initializers.WebUserAgentTelemetryInitializer"/> | ||
|
||
</TelemetryInitializers> |
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.
Suggest adding Tag so customers can see AI logs also in-case SDK behaves in a funky way.
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.
is it necessary to add a switch so that user could disable ai data collecting?
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.
The intention of this version of the sample is to show collecting AI data, so I don't feel that is necessary.
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.
@yungez I don't think adding a switch would make sense as the purpose of the demo app as Nicole said is to show the use case of multiple azure services in java with azure monitoring capabilities with AI.
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 don't have any major change requests. Just version number and Tag if added should be good to go.
@yungez @nicolehaugen79 is there any timeline when this PR is being merged into the AI branch as discussed? Without this merge AI integration with To-Do App is still incomplete correct? |
Add App Insights