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

mark Instrumentation Key APIs Obsolete #2592

Merged
merged 14 commits into from
May 24, 2022
Merged

Conversation

TimothyMothra
Copy link
Member

@TimothyMothra TimothyMothra commented May 5, 2022

#2560

Changes

  • mark PublicAPIs using Instrumentation Key as Obsolete.

image

@TimothyMothra TimothyMothra changed the title [WIP] mark Instrumentation Key obsolete mark Instrumentation Key APIs Obsolete May 10, 2022
@TimothyMothra TimothyMothra marked this pull request as ready for review May 10, 2022 22:00
{
}

/// <summary>
/// Initializes a new instance of the TelemetryConfiguration class.
/// </summary>
/// <param name="instrumentationKey">The instrumentation key this configuration instance will provide.</param>
[Obsolete("InstrumentationKey based global ingestion is being deprecated. Transition to using connection strings for data ingestion. https://aka.ms/MigrateToConnectionString")]
Copy link
Member Author

Choose a reason for hiding this comment

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

@cartersocha, need to review the messaging here

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

@@ -44,6 +44,7 @@ public static IApplicationBuilder UseApplicationInsightsExceptionTelemetry(this
/// <param name="services">The <see cref="IServiceCollection"/> instance.</param>
/// <param name="instrumentationKey">Instrumentation key to use for telemetry.</param>
/// <returns>The <see cref="IServiceCollection"/>.</returns>
[Obsolete("InstrumentationKey based global ingestion is being deprecated. Transition to using connection strings for data ingestion. https://aka.ms/MigrateToConnectionString")]
Copy link
Contributor

Choose a reason for hiding this comment

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

for an user of this API, this message is not telling how to migrate. The link is for an overall migration approach.

@cijothomas
Copy link
Contributor

@TimothyMothra I think we should offer specific alternate for each method we are Obsoleting.
For eg: service.AddAI(ikey) -- obsolete this, and suggest user to use service.AddAI(new AiOptions(){connectionstring}) ? Or create a github issue, document the migration there, and have obsolete message link to it. The github issue can further link to the aka.ms link to overall migration.

@TimothyMothra
Copy link
Member Author

@cijothomas I did both.
Updated the Obsolete message to point to our suggested alternative.
Also updated the linked issue with a full summary of these changes and as well as the relevant links to public docs.
Please re-review when you have a moment.

@@ -21,6 +21,7 @@ public static partial class ApplicationInsightsExtensions
/// <param name="services">The <see cref="IServiceCollection"/> instance.</param>
/// <param name="instrumentationKey">Instrumentation key to use for telemetry.</param>
/// <returns>The <see cref="IServiceCollection"/>.</returns>
[Obsolete("InstrumentationKey based global ingestion is being deprecated. Use an AddApplicationInsightsTelemetryWorkerService() overload to set ApplicationInsightsServiceOptions.ConnectionString. See https://github.com/microsoft/ApplicationInsights-dotnet/issues/2560 for more details.")]
Copy link
Contributor

Choose a reason for hiding this comment

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

the message is not very clear to me.
There is simply no other overload which takes ConnectionString. So suggesting to use an overload without being very clear which overload we are talking about can be confusing.

Copy link
Member Author

@TimothyMothra TimothyMothra May 23, 2022

Choose a reason for hiding this comment

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

I reviewed all the messages and added additional details.

I reworded this specific message to say Use the AddApplicationInsightsTelemetryWorkerService() overload which accepts Action<ApplicationInsightsServiceOptions> and set ApplicationInsightsServiceOptions.ConnectionString.

@TimothyMothra TimothyMothra merged commit e5b2ab0 into main May 24, 2022
@TimothyMothra TimothyMothra deleted the tilee/2560_ikeyobsolete branch May 24, 2022 19:27
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.

3 participants