Commit for #29 - Read version information from project.json #144
Conversation
Merge readme from develop to master
Use rc1-final instead of rc1-update1
|
Hi @gzepeda, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! TTYL, MSBOT; |
|
@gzepeda, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
|
Looks good to me! |
| { | ||
| private const string versionConfigurationOption = "dependencies:Microsoft.ApplicationInsights.AspNet"; | ||
|
|
||
| public ComponentVersionTelemetryInitializer(IHttpContextAccessor httpContextAccessor):base(httpContextAccessor) |
There was a problem hiding this comment.
Can you get IConfiguration object using dependency injection here and than try to do:
config[versionConfigurationOption].ToString();
instead of guessing which file contains configuration?
There was a problem hiding this comment.
Sure. I will try that approach.
|
I have made the change, but before creating a new Pull request. I wanted to ask if the approach was correct. I added this to the AddApplicationInsightsTelemetry method: And Injecting the IConfiguration dependency on the constructor for the created class. It does not conflict with the TelemetryConfiguration but I don't feel it is the right place to have that service declared. |
… version as requested in the previous pull request.
| @@ -48,6 +49,9 @@ public static void AddApplicationInsightsTelemetry(this IServiceCollection servi | |||
| services.AddSingleton<ITelemetryInitializer, WebSessionTelemetryInitializer>(); | |||
| services.AddSingleton<ITelemetryInitializer, WebUserTelemetryInitializer>(); | |||
|
|
|||
| var projectJsonConfiguration = new ConfigurationBuilder().AddJsonFile("project.json").Build(); | |||
There was a problem hiding this comment.
Configuration is already passed as an argument. You can pass it into initializer constructor
|
Done. One more thing: I am not sure how to reuse the service on the unit tests without declaring a new instance and sending it to the initializer. |
| @@ -48,6 +49,8 @@ public static void AddApplicationInsightsTelemetry(this IServiceCollection servi | |||
| services.AddSingleton<ITelemetryInitializer, WebSessionTelemetryInitializer>(); | |||
| services.AddSingleton<ITelemetryInitializer, WebUserTelemetryInitializer>(); | |||
|
|
|||
| services.AddInstance<IConfiguration>(config); | |||
There was a problem hiding this comment.
You can just pass this configuration to ComponentVersionTelemetryInitializer constructor on the line 44
There was a problem hiding this comment.
Should I do something like this:
services.AddSingleton<ITelemetryInitializer>(ComponentVersionTelemetryInitializer =>
{
return new ComponentVersionTelemetryInitializer(null, config);
});
The problem I see is not having the httpContextAccessor
There was a problem hiding this comment.
A bunch of Unit Tests produce exceptions if I send the first parameter (httpContextAccessor as null). Even if I don't need it on the initializer, as long as the class implements Interface TelemetryInitializerBase it is kind of mandatory to send it not null.
\test\Microsoft.ApplicationInsights.AspNet.Tests\Extensions\Appli
cationInsightsExtensionsTests.cs(52,0): at Microsoft.Extensions.DependencyInjection.ApplicationInsightsExtensionsTests.AddApplicationI
nsightsTelemetry.RegistersExpectedServices(Type serviceType, Type implementationType, ServiceLifetime lifecycle)
System.ArgumentNullException : Value cannot be null.
Parameter name: httpContextAccessor
\test\Microsoft.ApplicationInsights.AspNet.Tests\Extensions\Appli
cationInsightsExtensionsTests.cs(275,0): at Microsoft.Extensions.DependencyInjection.ApplicationInsightsExtensionsTests.AddApplication
InsightsSettings.RegistersTelemetryConfigurationFactoryMethodThatReadsInstrumentationKeyFromSettings()
System.ArgumentNullException : Value cannot be null.
Parameter name: httpContextAccessor
There was a problem hiding this comment.
Just inherit it from ITelemetryInitializer, not TelemetryInitializerBase
There was a problem hiding this comment.
Done. Let me know if the latest commits work.
… not needed in this context.
…lizer. - Removed not needed Unit tests. - Initialize dependency calling the constructor explicitly.
| @@ -14,7 +14,7 @@ | |||
| "delaySign": true | |||
| }, | |||
| "dependencies": { | |||
| "Microsoft.ApplicationInsights": "2.0.0-*", | |||
| "Microsoft.ApplicationInsights": "2.0.0-beta4", | |||
There was a problem hiding this comment.
Why are we specifically referring to beta4 here?
There was a problem hiding this comment.
Thank-you for all of your changes, unless there is a problem with 2.0.0-* please go ahead and change it and then we will complete the pull request.
Here are the changes for issue #29
There is a small change to the project.json file, I needed a dependency for Microsoft.Extensions.Configuration.Json and also change the version of Microsoft.ApplicationInsights so the project would compile.