-
Notifications
You must be signed in to change notification settings - Fork 67
remove correlation id lookup helper #880
remove correlation id lookup helper #880
Conversation
…lemented yet). Remove individual instances of Helper and instead depend on a singleton instance. Refactor tests to use single MockHelper class.
CHANGELOG.md
Outdated
@@ -1,5 +1,10 @@ | |||
# Changelog | |||
|
|||
## Version 2.6.0-beta4 | |||
- [Remove CorrelationIdLookupHelper. Use TelemetryConfiguration.ApplicationIdProvider instead.](https://github.com/Microsoft/ApplicationInsights-dotnet-server/pull/880) | |||
This change also fixes [ProfileQueryEndpoint is not overridable](https://github.com/Microsoft/ApplicationInsights-dotnet-server/issues/853) |
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'd suggest to formulate it as what you can do now, not what was the issue before. Instead "This change also fixes ProfileQueryEndpoint is not overridable" say "with this change you can update URL to query application ID from which enables environments with reverse proxy configuration to access Application Insights ednpoints".
@@ -7,4 +7,7 @@ | |||
<Add xdt:Transform="Remove" xdt:Locator="Match(Type)" Type="Microsoft.ApplicationInsights.DependencyCollector.DependencyTrackingTelemetryModule, Microsoft.AI.DependencyCollector" /> | |||
</TelemetryModules> | |||
<TelemetryModules xdt:Transform="Remove" xdt:Locator="Condition(count(*)=0)"/> | |||
|
|||
<ApplicationIdProvider xdt:Transform="Remove" xdt:Locator="Match(Type)" Type="Microsoft.ApplicationInsights.Extensibility.Implementation.ApplicationId.ApplicationInsightsApplicationIdProvider, Microsoft.ApplicationInsights"/> |
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.
Isn't it a default one? Do we need to set it explicitly?
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.
It's currently null by default.
Nothing uses it in the BaseSDK, but WebSDK would be expecting it.
…ub.com/Microsoft/ApplicationInsights-dotnet-server into tilee/remove_CorrelationIdLookupHelper
The most substantial change here is marking the former ProfileQueryEndpoint property as Obsolete to direct users to use the new TelemetryConfiguration property. I want to call attention to this and confirm that this is the correct procedure. |
TODO: Need to update all the configs on the functional tests |
…ub.com/Microsoft/ApplicationInsights-dotnet-server into tilee/remove_CorrelationIdLookupHelper
This is a continuation of the work started in BaseSDK: [New] Interface for ApplicationId Lookup (IApplicationIdProvider)
This ultimately fixes issue #853 where the Endpoint for CorrelationId could not be customized.
RequestTelemetryModule and DependencyTelemetryModule will now rely on a configuration level ApplicationIdProvider to resolve Ids used for correlation. This provider will be configured with default settings on Nuget install.
For significant contributions please make sure you have completed the following items:
Changes in public surface reviewed
Design discussion issue #
CHANGELOG.md updated with one line description of the fix, and a link to the original issue.
The PR will trigger build, unit tests, and functional tests automatically. If your PR was submitted from fork - mention one of committers to initiate the build for you.
If you want to to re-run the build/tests, the easiest way is to simply Close and Re-Open this same PR. (Just click 'close pull request' followed by 'open pull request' buttons at the bottom of the PR)
Please follow [these] (https://github.com/Microsoft/ApplicationInsights-dotnet-server/blob/develop/CONTRIBUTING.md) instructions to build and test locally.