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.

Sign up for GitHub

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

Jump to bottom

refactor TransmissionPolicy #2311

Merged
merged 8 commits into from
Jun 24, 2021
Merged

refactor TransmissionPolicy #2311

merged 8 commits into from
Jun 24, 2021

Conversation

TimothyMothra
Copy link
Member

Here I've refactored TransmissionPolicy and introduced a new class TransmissionPolicyCollection.

Why
I need to introduce a new TransmissionPolicy for AAD scenarios.
Moving all the existing policies into their own collection let me review how they are used.

Changes

  • Introduced a new class TransmissionPolicyCollection to encapsulate all policy classes and their interactions.
  • Moved existing classes to a new namespace.

Checklist

  • I ran Unit Tests locally.
  • CHANGELOG.md updated with one line description of the fix, and a link to the original issue if available.

For significant contributions please make sure you have completed the following items:

  • Design discussion issue #
  • Changes in public surface reviewed

The PR will trigger build, unit tests, and functional tests automatically. Please follow these instructions to build and test locally.

Notes for authors:

  • FxCop and other analyzers will fail the build. To see these errors yourself, compile localy using the Release configuration.

Notes for reviewers:

  • We support comment build triggers
    • /AzurePipelines run will queue all builds
    • /AzurePipelines run <pipeline-name> will queue a specific build

cijothomas
cijothomas reviewed Jun 22, 2021
View reviewed changes
...TelemetryChannel/Implementation/TransmissionPolicy/ApplicationLifecycleTransmissionPolicy.cs
@@ -1,4 +1,4 @@
namespace Microsoft.ApplicationInsights.WindowsServer.TelemetryChannel.Implementation
namespace Microsoft.ApplicationInsights.WindowsServer.TelemetryChannel.Implementation.TransmissionPolicy
Copy link
Contributor

Choose a reason for hiding this comment

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

dont think its really worthy as these are internals anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

We have 5 classes that inherit TransmissionPolicy. (I'm adding a 6th in my new PR).
I had a hard time finding the existing clrasses because they were all in the root.

I've moved them all to a unique subfolder and updated the namespace to match.

@cijothomas
Copy link
Contributor

I need to introduce a new TransmissionPolicy for AAD scenarios.
Moving all the existing policies into their own collection let me review how they are used.

Not sure how this refactoring is going to help adding a new TransmissionPolicy for AAD!

@TimothyMothra
Copy link
Member Author

I need to introduce a new TransmissionPolicy for AAD scenarios.
Moving all the existing policies into their own collection let me review how they are used.

Not sure how this refactoring is going to help adding a new TransmissionPolicy for AAD!

I'm planning on switching an AuthenticationTransmissionPolicy on or off based on if the Credential has been configured.
I can do this without the refactor, but it adds more private vars to the ServerTelemetryChannel.

Secondly, I wasn't certain of how all the Policies are used - I have a better understanding of that now because of this PR.
It made sense to me to encapsulate our existing policies into a single class.

rajkumar-rangaraj
rajkumar-rangaraj approved these changes Jun 23, 2021
View reviewed changes
Copy link
Member

@rajkumar-rangaraj rajkumar-rangaraj left a comment

Choose a reason for hiding this comment

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

LGTM

@TimothyMothra TimothyMothra enabled auto-merge (squash) June 23, 2021 23:56
@TimothyMothra TimothyMothra merged commit 066e180 into develop Jun 24, 2021
@TimothyMothra TimothyMothra deleted the tilee/refactor_transmissionpolicy branch June 24, 2021 00:25
tokaplan added a commit to tokaplan/ApplicationInsights-dotnet that referenced this pull request Jul 22, 2021
@tokaplan @rajkumar-rangaraj
@cijothomas @TimothyMothra @pharring @OskarKlintrot @ank3it @JamesNK @dependabot @xiang17 @marodev @pakrym @LevYas
Merge from main into fork (#1)
52c8653 
* Added support for FlushAsync API

* Added more test cases

* Corrections to test case

* fixed merge conflicts

* Fixed an issue with IsEnqueueSuccess

* Bumping CurrentInvariantVersion for QuickPulse. (microsoft#2123)

* Endpoint and ping interval hints for QuickPulseModule.

* Updating changelog.md with the PR link for the change.

* StyleCop error fix.

* Bumping CurrentInvariantVersion for QuickPulse.

* Unit test fixes.

* Update how BuildNumbers are calculated (microsoft#2125)

* Update _GlobalStaticVersion.props

Change to the way we calculate the build number.

Formerly, we calculated builds as the TotalMinutes between DateTime.Now and the semantic version time stamp, divided by 5.
This meant that every 5 minutes the build number changes.
This was hurting nightly nupkgs because our assembly version and packages weren't matching.

This PR changes the calculation to be TotalHours / 12.

* Update release_NupkgAudit.ps1

* Update Changelog, prep 2.17.0-beta1 (microsoft#2126)

* Update release_NupkgAudit.ps1 (microsoft#2128)

* Update release_NupkgAudit.ps1

removing the hardcoded hash.
will put the value in the build definition. This will be easier to update when certs rotate.

* Update release_NupkgAudit.ps1

punctuation

* upgrade log4net 2.0.10 (microsoft#2150)

* Fix: telemetry parent id when using W3C activity format (microsoft#2145)

* Fix telemetry parent id when using W3C activity format

* Add unit tests

* Added InFlightTransmission struct

* Add response duration to TransmissionStatusEventArgs

* Update ChangeLog

* Update profiler and snapshot endpoints
Fixes microsoft#2166

* bump version 2.17 Stable (microsoft#2171)

* bump version

* Update CHANGELOG.md

* finalize public API

* remove outdated NuGet.Config files

* Update CONTRIBUTING.md (microsoft#2177)

* remove comment

* Update Readme.md (microsoft#2182)

Adding Support policy to readme

* Update bug_report.md (microsoft#2183)

* Update bug_report.md

Adding Immediate support policy to the bug template

* Removing item check in serializer

* Fix tests

* New Implementation

* Added Increment to pause storage dequeue

* thread sync support for move transmissions

* Create dependabot.yml (microsoft#2201)

* Fix PropertyFetcher error when used with multiple types (microsoft#2197)

* Fix PropertyFetcher error when used with multiple types

* Fix build errors

Co-authored-by: Timothy Mothra <tilee@microsoft.com>

* remove Microsoft.TestPlatform.TestHost (microsoft#2208)

* Bump Microsoft.NET.Test.Sdk from 16.7.1 to 16.9.4 (microsoft#2206)

* Bump Microsoft.NET.Test.Sdk from 16.7.1 to 16.9.4

Bumps [Microsoft.NET.Test.Sdk](https://github.com/microsoft/vstest) from 16.7.1 to 16.9.4.
- [Release notes](https://github.com/microsoft/vstest/releases)
- [Commits](microsoft/vstest@v16.7.1...v16.9.4)

Signed-off-by: dependabot[bot] <support@github.com>

* Update Microsoft.ApplicationInsights.Isolated.Tests.csproj

remove System.Reflection.Metadata

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Timothy Mothra <tilee@microsoft.com>

* Bump NETStandard.HttpListener from 1.0.2 to 1.0.3.5 (microsoft#2205)

Bumps [NETStandard.HttpListener](https://github.com/StefH/NETStandard.HttpListener) from 1.0.2 to 1.0.3.5.
- [Release notes](https://github.com/StefH/NETStandard.HttpListener/releases)
- [Commits](https://github.com/StefH/NETStandard.HttpListener/commits)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump Newtonsoft.Json from 10.0.3 to 13.0.1 (microsoft#2209)

* Bump Newtonsoft.Json from 10.0.3 to 13.0.1

Bumps [Newtonsoft.Json](https://github.com/JamesNK/Newtonsoft.Json) from 10.0.3 to 13.0.1.
- [Release notes](https://github.com/JamesNK/Newtonsoft.Json/releases)
- [Commits](JamesNK/Newtonsoft.Json@10.0.3...13.0.1)

Signed-off-by: dependabot[bot] <support@github.com>

* Update Microsoft.ApplicationInsights.Tests.csproj

* Update Microsoft.ApplicationInsights.Isolated.Tests.csproj

* Update TelemetryChannel.Nuget.Tests.csproj

* Update TelemetryChannel.Tests.csproj

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Timothy Mothra <tilee@microsoft.com>

* Bump log4net from 2.0.10 to 2.0.12 (microsoft#2204)

Bumps [log4net](https://github.com/apache/logging-log4net) from 2.0.10 to 2.0.12.
- [Release notes](https://github.com/apache/logging-log4net/releases)
- [Changelog](https://github.com/apache/logging-log4net/blob/master/ReleaseInstructions.txt)
- [Commits](apache/logging-log4net@rel/2.0.10...rc/2.0.12)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump System.Text.Encoding.CodePages from 4.3.0 to 5.0.0 (microsoft#2211)

* Bump Microsoft.AspNetCore.Identity from 2.1.1 to 2.1.2 (microsoft#2215)

* Bump Microsoft.AspNetCore.StaticFiles from 2.1.1 to 2.2.0 (microsoft#2216)

* Bump Microsoft.AspNetCore.Mvc.TagHelpers from 2.1.1 to 2.2.0 (microsoft#2212)

* update and consolidate xunit dependencies (microsoft#2222)

* update and consolidate xunit dependencies

* fix xunit analyzer

* Bump MSTest.TestAdapter from 2.1.2 to 2.2.3 (microsoft#2223)

* Bump System.Console from 4.3.0 to 4.3.1 (microsoft#2224)

* Bump Microsoft.AspNetCore.Server.IISIntegration from 2.1.1 to 2.2.1 (microsoft#2226)

* Saving work in progress.

* API changes

* Additional tests

* spaces

* Review changes

* Fix flaky test.

* PR feedback

* InternalsVisibleTo Microsoft.AI.ServerTelemetryChannel (microsoft#2229)

* Add CancellationToken check for MoveTransmissions

* Bump Microsoft.AspNetCore.Server.Kestrel from 1.1.2 to 2.2.0 (microsoft#2207)

* Bump System.Data.SqlClient from 4.7.0 to 4.8.2 (microsoft#2231)

Bumps [System.Data.SqlClient](https://github.com/dotnet/corefx) from 4.7.0 to 4.8.2.
- [Release notes](https://github.com/dotnet/corefx/releases)
- [Commits](https://github.com/dotnet/corefx/commits)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* examples sln (microsoft#2233)

* new solution

* fix path

* additional properties in props

* adding AspNetCore WebApp to Examples

* simplify property

* PR feedback

* Initialization code and entry point for Self Diagnostics Module

* Adding access modifier

* Add APIs as declared API list

* Fix flaky test - FlushAsyncTransmissionWithThrottle (microsoft#2247)

* Add ManualResetEventSlim.Wait

* Fix warning

* general maintenance (microsoft#2250)

* general maintenance

* fix date

* update analyzers (microsoft#2198)

* update analyzers

* remove default case, tests should pass

* fxcop

* cleanup

* testing fix

* remove net452 and net46 from aspnetcore (microsoft#2252)

* remove net452 and net46 from aspnetcore

* changelog

* consolidate package references

* cleanup unnecessary dependencies in AspNetCore SDK (microsoft#2253)

* Move location of Initialization

* [SDL] update packages (microsoft#2243)

* update packages

* update test dependencies

* testing fix for version conflicts

* Update IntegrationTests.Tests.csproj

* cleanup

* cleanup

* Remove "Module" in class name. Change class to internal.

* Update CHANGELOG.md (microsoft#2254)

* Add config parser class for SelfDiagnostics

* Rename SelfDiagnostics class to SelfDiagnosticsInitializer, and SelfDiagnosticsInternals names to SelfDiagnostics.

* Rename test class namespace. Remove xunit dependency.

* Add EventListener for SelfDiagnostics (microsoft#2259)

* Add EventListener for SelfDiagnostics

* Match with EventSource name's prefix

* troubleshooting (microsoft#2264)

* troubleshooting guides

* update

* update

* update

* Update Readme.md

* Update Readme.md

* Update PostTelemetry.ps1

* Update Readme.md

* Encode event and write to file in SelfDiagnosticsEventListener (microsoft#2265)

* Encode event and write to file in SelfDiagnosticsEventListener

* Fix compile error

* Add MemoryMappedFileHandler for SelfDiagnostics (microsoft#2258)

* Add MemoryMappedFileHandler for SelfDiagnostics

* Rename classes and namespace.

* -

* Use CollectionAssert.AreEqual to compare byte[]

* Add circular write logic into MemoryMappedFileHandler class

* Fix trivial compile errors

* Fix trivial compile errors

* Update EventSource method implementation

* Update EventSource method implementation

* -

* Special requirement for last argument in method definition and last parameter for WriteEvent call

* update typo in comment

* Add ConfigRefresher for SelfDiagnostics (microsoft#2262)

* Add MemoryMappedFileHandler for SelfDiagnostics

* Rename classes and namespace.

* -

* Use CollectionAssert.AreEqual to compare byte[]

* Add circular write logic into MemoryMappedFileHandler class

* Add ConfigRefresher for SelfDiagnostics

* Fix trivial compile errors

* Fix trivial compile errors

* Update EventSource method implementation

* Update EventSource method implementation

* -

* Special requirement for last argument in method definition and last parameter for WriteEvent call

* Add new line at end of file

* Change namespace for SelfDiagnosticsInitializer

* Remove redundant using namespace statement.

* Remove blank line

* Cosmetics/technical debt  - use pattern matching (microsoft#2268)

* Fix ReSharper's "Convert 'as' expression type check and the following null check into pattern matching" / Code Smell

* Update changelog with description

* Update Readme and changelog for SelfDiagnostics (microsoft#2267)

* Add Readme and changelog for Self Diagnostics

* Fix failed test cases

* Add changelog entry

* Update troubleshooting/ETW/Readme.md with Self Diagnostics section

* Remove unused code

* Add "as of version 2.18.0"

* Update Self-Diagnostics instructions (microsoft#2271)

* Update Self-Diagnostics instructions

* Update Readme.md

* Update Readme.md

* add Net5.0 to Test Infra and Bask SDK Tests (microsoft#2272)

* add support for net461 to Tests (microsoft#2274)

* fix Test projects dependency, Microsoft.AspNetCore.App (microsoft#2276)

* cleanup frameworks in AspNetCore (microsoft#2278)

* cleanup frameworks in AspNetCore

* fix fxcop

* remove unused dependency (microsoft#2281)

* AAD: Handling Azure.Core.TokenCredential (microsoft#2191)

* Bump Microsoft.AspNetCore.Mvc.Core from 1.0.3 to 1.0.4 (microsoft#2285)

Bumps Microsoft.AspNetCore.Mvc.Core from 1.0.3 to 1.0.4.

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* disable flaky tests (microsoft#2289)

* AAD: refactor (microsoft#2288)

* AAD: refactor

* change property to internal

* AAD: with InMemoryChannel (microsoft#2290)

* aad with InMemoryChannel

* Fix AzureSdkDiagnosticListener from crashing user app (microsoft#2294)

* Fix AzureSdkDiagnosticListener from crashing user app

* changelog

* Read Azure SDK Success status (microsoft#2200)

* Read Azure SDK Success status

* cleanup Base SDK Frameworks (microsoft#2280)

Co-authored-by: Cijo Thomas <cithomas@microsoft.com>

* AAD: with QuickPulse (microsoft#2291)

* AAD: with QuickPulse

* AAD: with ServerTelemetryChannel (microsoft#2292)

* AAD: with ServerTelemetryChannel

* bump version 2.18-beta2 (microsoft#2296)

* disable these tests on linux (microsoft#2298)

* Updated link to performance counters docs (microsoft#2301)

* Updated link to performance counters docs

* Update WEB/Src/PerformanceCollector/README.md

Co-authored-by: Cijo Thomas <cithomas@microsoft.com>

* aad: detect type (microsoft#2303)

* Enable the self diagnostics and fix a NullReferenceException bug (microsoft#2302)

* Enable the self diagnostics and fix a NullReferenceException bug

* Fix compilation warnings.

* enable self-diagnostics in example app (microsoft#2305)

* AAD: change to CredentialEnvelope to expose Expiration  (microsoft#2306)

* refactor TransmissionPolicy (microsoft#2311)

* Remove OpenCover. CVE-2018-1285 (microsoft#2313)

* Update Microsoft.Extensions.Logging to 2.1.1

* AAD: new AuthenticationTransmissionPolicy for retry scenarios. (microsoft#2312)

* update version for beta3 (microsoft#2316)

* AAD: Misc changes (microsoft#2317)

* initialize cache inside constructor

* remove todo. not pursing caching right now

* cleanup comment

* update changelog

* Update ReflectionCredentialEnvelope.cs

* log to indicated Authentication Policy caught the response from ingestion (microsoft#2319)

* Update linux-build.yml (microsoft#2329)

update version of dotnet 5

* Self-Diagnostics: include datetimestamp in filename (microsoft#2325)

* include datetimestamp in self-diagnostics filename

* come review comments

* update readme

* update readme

* Update Readme.md

* Update CHANGELOG.md (microsoft#2332)

* prune public api (microsoft#2336)

* prep 2.18 (microsoft#2337)

* Tilee/examples (microsoft#2339)

* move example projects to root. change to project reference

* update packages

* remove release config

* rename projects

* build definition for examples solution

* cleanup yml

* fix yml

* test fix for yml

* test fix yml

* Update examples-sanity.yml (microsoft#2340)

correct branch name

Co-authored-by: Rajkumar Rangaraj <rajrang@microsoft.com>
Co-authored-by: Cijo Thomas <cithomas@microsoft.com>
Co-authored-by: Timothy Mothra <tilee@microsoft.com>
Co-authored-by: ylabade <64366368+ylabade@users.noreply.github.com>
Co-authored-by: Paul Harrington <pharring@microsoft.com>
Co-authored-by: Paul Harrington <pharring@users.noreply.github.com>
Co-authored-by: Oskar Klintrot <oskar.klintrot@gmail.com>
Co-authored-by: ank3it <anksr@microsoft.com>
Co-authored-by: James Newton-King <james@newtonking.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: xiang17 <xili9@microsoft.com>
Co-authored-by: Martin <modermatt@tuta.io>
Co-authored-by: Pavel Krymets <pavel@krymets.com>
Co-authored-by: Lev Yastrebov <36070899+LevYas@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cijothomas cijothomas cijothomas left review comments

@rajkumar-rangaraj rajkumar-rangaraj rajkumar-rangaraj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@TimothyMothra @cijothomas @rajkumar-rangaraj