-
Notifications
You must be signed in to change notification settings - Fork 798
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
feat(Publisher.ApplicationInsights): add connection string support #1480
feat(Publisher.ApplicationInsights): add connection string support #1480
Conversation
…ection string support
…ection string support
...r.ApplicationInsights/DependencyInjection/ApplicationInsightsHealthCheckBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/HealthChecks.Publisher.ApplicationInsights/ApplicationInsightsPublisher.cs
Show resolved
Hide resolved
src/HealthChecks.Publisher.ApplicationInsights/ApplicationInsightsPublisher.cs
Outdated
Show resolved
Hide resolved
…ghtsPublisher.cs Co-authored-by: Ivan Maximov <[email protected]>
/// <param name="saveDetailedReport">Specifies if save an Application Insights event for each HealthCheck or just save one event with the global status for all the HealthChecks. Optional</param> | ||
/// <param name="excludeHealthyReports">Specifies if save an Application Insights event only for reports indicating an unhealthy status</param> | ||
/// <returns>The specified <paramref name="builder"/>.</returns> | ||
public static IHealthChecksBuilder AddApplicationInsightsPublisher( | ||
this IHealthChecksBuilder builder, | ||
string? connectionString = default, |
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.
This change is breaking. Both at binary level and source level - you should point compiler what arg to pass - instrumentationKey:
or connectionString:
. For someone it can be a problem though I don't see a way to do better decision.
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.
So, can we merge this PR?
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.
Maybe after migrating repo to .NET 7 SDK and bump major version? @carlosrecuero ?
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.
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.
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 synced my fork :)
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.
Thanks. Waiting for CI.
I added label. |
…sher.ApplicationInsights/connection-string-support
Will be released as a part of #1593 . |
What this PR does / why we need it:
On March 21, 2025 Application Insights will deprecate instrumentation key based data ingestion (azure-deprecation/dashboard#212).
AspNetcore.HealthChecks.Publisher.ApplicationInsights
supportsinstrumentationKey
as configuration option forTelemetryClient
.Microsoft suggests to migrate to connection string-based authentication instead.
Which issue(s) this PR fixes:
azure-deprecation/dashboard#212
Special notes for your reviewer:
Please add the label
hacktoberfest-accepted
to this pull request, so that this pull requests counts as contribution towards https://hacktoberfest.com .Does this PR introduce a user-facing change?:
Yes, it introduces a new optional parameter
connectionString
in the signature of the methodAddApplicationInsightsPublisher(...)
.Please make sure you've completed the relevant tasks for this PR, out of the following list:
Sample with connection string: