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

feat(Publisher.ApplicationInsights): add connection string support #1480

Merged
merged 5 commits into from
Dec 29, 2022
Merged

feat(Publisher.ApplicationInsights): add connection string support #1480

merged 5 commits into from
Dec 29, 2022

Conversation

skyfrk
Copy link
Contributor

@skyfrk skyfrk commented Oct 8, 2022

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 supports instrumentationKey as configuration option for TelemetryClient.
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 method AddApplicationInsightsPublisher(...).

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Unit tests passing
  • End-to-end tests passing
  • Extended the documentation <-- not needed, I guess
  • Provided sample for the feature

Sample with connection string:

public void ConfigureServices(IServiceCollection services)
{
    services
        .AddHealthChecks()
        .AddApplicationInsightsPublisher(connectionString: "InstrumentationKey=00000000-0000-0000-0000-000000000000;IngestionEndpoint=https://custom.com:111/;LiveEndpoint=https://custom.com:222/;ProfilerEndpoint=https://custom.com:333/;SnapshotEndpoint=https://custom.com:444/;");
}

/// <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,
Copy link
Collaborator

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.

Copy link
Contributor Author

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?

Copy link
Collaborator

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 ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

@skyfrk please pull changes from master to verify CI. Also now is a good time to go on with any breaking changes if any. See #1574.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I synced my fork :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks. Waiting for CI.

@sungam3r sungam3r added the hacktoberfest-accepted This PR counts as contribution towards https://hacktoberfest.com label Oct 26, 2022
@sungam3r
Copy link
Collaborator

I added label.

…sher.ApplicationInsights/connection-string-support
@sungam3r sungam3r merged commit c5af010 into Xabaril:master Dec 29, 2022
@sungam3r
Copy link
Collaborator

Will be released as a part of #1593 .

@sungam3r sungam3r mentioned this pull request Jul 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted This PR counts as contribution towards https://hacktoberfest.com
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants