Skip to content
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

Obsolete TelemetryConfiguration.Active singleton usage on .NET Core #1148

Merged
merged 8 commits into from
Jun 13, 2019

Conversation

lmolkova
Copy link
Member

@lmolkova lmolkova commented May 23, 2019

We have a stream of issues and support requests coming from confusion around TelemetryConfiguration.Active on ASP.NET Core.

We DO NOT recommend using TelemetryConfiguration.Active - instead we recommend using DI from Microsoft.Extensions.DependencyInjection.

In the case of ASP.NET Core or generic host applications, here is an instruction on how to use/configure TelemetryConfiguration: https://docs.microsoft.com/en-us/azure/azure-monitor/app/asp-net-core

This change makes Active obsolete on .NET Core to further discourage its usage there.

@cijothomas
Copy link
Contributor

@lmolkova I wonder what would be our suggestion for .NET Core Console Apps which don't use DI, so using .Active probably okay there?
How can we handle this in code? we cannot detect .NET Core vs Asp.Net Core, and apply Obsolete tag.

@lmolkova
Copy link
Member Author

lmolkova commented May 23, 2019

I wonder what would be our suggestion for .NET Core Console Apps which don't use DI, so using .Active probably okay there?
How can we handle this in code? we cannot detect .NET Core vs Asp.Net Core, and apply Obsolete tag.

Console applications should carry all objects explicitly.
They should create the configuration with TelemetryConfiguration.CreateDefault() and carry it everywhere as any other object they create (e.g. DB client or Azure Storage client).

They can install DI extensions from Microsoft.Extensions.DependencyInjection and use DI pattern or use any other DI framework. They also can explicitly pass configuration/client around.

If they really want static singleton - it is their choice to store explicitly created instance in global static variable, but we should not promote it or do it ourselves.

If we decide to proceed with this change, we should update console app docs (and monitor other docs) to reflect this.
@Dmitry-Matveev @SergeyKanzhelev I hope to hear your opinion on this.

@Dmitry-Matveev
Copy link
Member

@lmolkova , if docs are to be updated with the statements you outlined, I'm OK deprecating this in .NET Core branch.

Copy link
Contributor

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

I am okay with this change, but please ensure docs are updated by the time we ship beta1 with this change.
Other SDK repos will start showing compile errors for using things deprecated from this change. so need to change those as well.

"We do not recommend using TelemetryConfiguration.Active on .NET Core. " +
"Instead we suggest using dependency injection pattern. " +
"See https://docs.microsoft.com/en-us/aspnet/core/fundamentals/dependency-injection?view=aspnetcore-2.2 " +
"and https://docs.microsoft.com/en-us/azure/azure-monitor/app/asp-net-core for more info")]
Copy link
Contributor

Choose a reason for hiding this comment

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

its best to create a new issue in Github repo, and link to the above links from there. Main reason is - the above links are all 'asp.net core' specific, not so much about '.net core console'. So our Github issue can act as a wrapper, and have links to asp.net core with di, .net core with di, or just plain .net core console etc. (if needed).

Also it'd be good place for us to explain the rationale behind this decision in a github issue in this repo.

@lmolkova lmolkova merged commit 6532148 into develop Jun 13, 2019
@lmolkova lmolkova deleted the lmolkova/ActiveObsoleteOnCore branch June 13, 2019 20:09
idrivediesel2006 added a commit to idrivediesel2006/serilog-sinks-applicationinsights that referenced this pull request Jul 26, 2022
According to Microsoft in this PR (microsoft/ApplicationInsights-dotnet#1148) they recommend using Telemetry.CreateDefault().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants