Make all built-in TelemetryInitializers public to enable easy removal#611
Make all built-in TelemetryInitializers public to enable easy removal#611cijothomas merged 3 commits intodevelopfrom
Conversation
… without using reflection magic.
|
@pakrym Please let me know if we have a strong reason now to keep this internal. @pharring Please take a look as I see you were active in the original issues posted about this. |
…eed to revisit later.
|
Looks good to me |
| /// on telemetries. | ||
| /// </summary> | ||
| internal class AspNetCoreEnvironmentTelemetryInitializer: ITelemetryInitializer | ||
| public class AspNetCoreEnvironmentTelemetryInitializer: ITelemetryInitializer |
There was a problem hiding this comment.
(This comment applies to all the newly public classes.)
Should these types be sealed? It's usually a good idea with public types that were not designed to be extended by users - especially if they have any virtual methods.
You can always unseal a public type later, but you can't go the other way 'round.
There was a problem hiding this comment.
I agree it can be made sealed. However, similar ones in Web repo are not sealed.
I'll limit my change to just making classes public for now. When we address (#612) this can be revisited.
| /// A telemetry initializer that will gather Azure Web App Role Environment context information. | ||
| /// </summary> | ||
| internal class AzureWebAppRoleEnvironmentTelemetryInitializer : TelemetryInitializerBase | ||
| public class AzureWebAppRoleEnvironmentTelemetryInitializer : TelemetryInitializerBase |
There was a problem hiding this comment.
My comment about sealing applies more-so to those derived from TelemetryInitializerBase
|
Thanks all for quick review. Will address Pauls comment about sealing these classes when we address #612 |
4 participants
TelemetryInitializers being internal makes it difficult to remove an individual one. (need reflection magic to achieve this currently)
Fix Issue # .
#351
#346
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-aspnetcore/blob/develop/Readme.md) instructions to build and test locally.