Skip to content
This repository has been archived by the owner on Jul 5, 2020. It is now read-only.

remove correlation id lookup helper #880

Merged
merged 26 commits into from
Apr 16, 2018

Conversation

TimothyMothra
Copy link
Member

@TimothyMothra TimothyMothra commented Apr 9, 2018

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.

  • I ran Unit Tests locally.

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.

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)
Copy link
Contributor

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"/>
Copy link
Contributor

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?

Copy link
Member Author

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.

@TimothyMothra
Copy link
Member Author

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.

@TimothyMothra
Copy link
Member Author

TODO: Need to update all the configs on the functional tests

@TimothyMothra TimothyMothra merged commit 8645f55 into develop Apr 16, 2018
@TimothyMothra TimothyMothra deleted the tilee/remove_CorrelationIdLookupHelper branch April 17, 2018 21:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants