Create CollectDump Collection Rule Action #824

kkeirstead · 2021-09-03T20:11:41Z

Includes one basic test for file system egress; open to including more if other tests are desired.

Closes #632

…ay be close to functional, but don't yet have tests to confirm that.

…uring out what needs to be pulled from existing tests and how to combine it to make a test that can collect a dump using ExecuteAsync.

…. Might end up waiting until Justin's change gets merged in for using IHost to get this resolved.

… the .dmp suffix and not entirely sure whether it's working correctly, but exciting to see it function. Code is currently a mess and needs to be refined and trimmed.

…hat the permanent one is not being written. Need to investigate how that actually happens.

…rmediary check in

…onitor into kkeirstead/triggers/DumpAction

…about how to best integrate with existing infrastructure.

…as hurting readability in tests. No functional changes.

…cified directory. Still need to do refactoring to better share code, and probably another test for azure blob egress.

…d egress provider due to built-in validation that is not used when running unit tests. Should enter PR soon.

…r unknown reasons.

…t build because of complaints about netcore2.1

kkeirstead · 2021-09-03T20:12:20Z

src/Tests/Microsoft.Diagnostics.Monitoring.Tool.FunctionalTests/DumpTests.cs

@@ -66,73 +58,7 @@ public Task DumpTest(DiagnosticPortConnectionMode mode, DumpType type)
                    using ResponseStreamHolder holder = await client.CaptureDumpAsync(processId, type);
                    Assert.NotNull(holder);

-                    byte[] headerBuffer = new byte[64];


This has been moved to IDumpTestInterface

wiktork · 2021-09-03T21:06:09Z

src/Tools/dotnet-monitor/CollectionRules/Actions/CollectDumpAction.cs

+
+            IProcessInfo processInfo = await _diagnosticServices.GetProcessAsync(new ProcessKey(endpointInfo.ProcessId), token);
+
+            string dumpFileName = DiagController.GenerateDumpFileName();


Avoid this kind of coupling. Consider a separate utility class for generating the name rather than making Dump actions depend on WebServer controllers.

wiktork · 2021-09-03T21:06:32Z

src/Tools/dotnet-monitor/CollectionRules/Actions/CollectDumpAction.cs

+
+            string dumpFileName = DiagController.GenerateDumpFileName();
+
+            string dumpFilePath = "";


nit: string.Empty

wiktork · 2021-09-03T21:10:59Z

src/Tests/Microsoft.Diagnostics.Monitoring.Tool.UnitTests/EndpointInfoSourceTests.cs

@@ -214,7 +214,7 @@ public static IEnumerable<object[]> GetTfmsSupportingPortListener()
            yield return new object[] { TargetFrameworkMoniker.Net60 };
        }

-        private ServerEndpointInfoSource CreateServerSource(out string transportName, ServerEndpointInfoCallback callback = null)
+        internal ServerEndpointInfoSource CreateServerSource(out string transportName, ServerEndpointInfoCallback callback = null)


If these are useful as utility methods, refactor them to a separate class that can be used by multiple test classes. Test classes should not be calling to each other. Then the utility class can have public methods that are clearly meant to be consumed by other code.

wiktork · 2021-09-03T21:13:35Z

src/Tests/Microsoft.Diagnostics.Monitoring.Tool.FunctionalTests/IDumpTestInterface.cs

+
+namespace Microsoft.Diagnostics.Monitoring.Tool.FunctionalTests
+{
+    public interface IDumpTestInterface


I think what you really want here is a static class.

wiktork · 2021-09-03T21:21:47Z

src/Tests/Microsoft.Diagnostics.Monitoring.Tool.UnitTests/TestHostHelper.cs

                {
                    services.ConfigureCollectionRules();
                    services.ConfigureEgress();
+
+                    services.AddSingleton<EgressOperationQueue>();


Do we really need these dependencies? You are egressing directly from what I can see.

wiktork · 2021-09-03T21:24:14Z

src/Tools/dotnet-monitor/CollectionRules/Actions/CollectDumpAction.cs

+
+                try
+                {
+                    dumpFilePath = await SendToEgress(new EgressOperation(


Since we are going to have to egress from many other actions, it would be very useful if we had an egress facade that could be used by every action.

src/Tests/Microsoft.Diagnostics.Monitoring.Tool.FunctionalTests/IDumpTestInterface.cs

...Diagnostics.Monitoring.Tool.UnitTests/Microsoft.Diagnostics.Monitoring.Tool.UnitTests.csproj

src/Tools/dotnet-monitor/CollectionRules/Actions/CollectDumpAction.cs

src/Tests/Microsoft.Diagnostics.Monitoring.Tool.UnitTests/TestHostHelper.cs

src/Tools/dotnet-monitor/CollectionRules/Actions/CollectDumpAction.cs

…should be consistent with the initial commit.

kkeirstead · 2021-09-07T22:50:37Z

Currently still dealing with additional merge conflicts that are causing test failures; will push again once resolved.

…unding EndpointInfoSourceCallback and EndpointUtilities). Local tests are passing.

…nto the Utilities class.

…keirstead/dotnet-monitor into kkeirstead/triggers/DumpAction

…leNotFound exceptions, so made some changes that bring this code to more closely resemble what's being done in the Egress tests. Tests still pass locally.

src/Microsoft.Diagnostics.Monitoring.WebApi/DumpService.cs

src/Microsoft.Diagnostics.Monitoring.WebApi/IDiagnosticServices.cs

...crosoft.Diagnostics.Monitoring.TestCommon/Microsoft.Diagnostics.Monitoring.TestCommon.csproj

src/Tools/dotnet-monitor/DiagnosticsMonitorCommandHandler.cs

src/Tests/Microsoft.Diagnostics.Monitoring.Tool.UnitTests/CollectDumpActionTests.cs

wiktork · 2021-09-10T17:19:27Z

src/Microsoft.Diagnostics.Monitoring.WebApi/Utilities.cs

+
+namespace Microsoft.Diagnostics.Monitoring.WebApi
+{
+    public class Utilities


internal static class.

wiktork · 2021-09-10T17:22:36Z

src/Microsoft.Diagnostics.Monitoring.WebApi/Utilities.cs

+        public const string ArtifactType_Trace = "trace";
+        public const string ArtifactType_Metrics = "collectmetrics";
+
+        internal static string GenerateDumpFileName()


Imo, internal classes with public methods are much better for this:

It is clear the methods are meant to be consumed by any caller but the class as a whole is an implementation detail

It gives room for true internal methods. Things designed to be called by tests or glue code between classes that shouldn't be called by just anyone

It makes it very easy to switch to a public api should that ever be desired.

src/Tools/dotnet-monitor/CollectionRules/Actions/CollectDumpAction.cs

src/Microsoft.Diagnostics.Monitoring.WebApi/DumpService.cs

...crosoft.Diagnostics.Monitoring.TestCommon/Microsoft.Diagnostics.Monitoring.TestCommon.csproj

src/Tests/Microsoft.Diagnostics.Monitoring.Tool.UnitTests/CollectDumpActionTests.cs

src/Tests/Microsoft.Diagnostics.Monitoring.Tool.UnitTests/EndpointInfoSourceTests.cs

src/Tools/dotnet-monitor/CollectionRules/Actions/CollectDumpAction.cs

src/Microsoft.Diagnostics.Monitoring.WebApi/DumpService.cs

src/Tests/Microsoft.Diagnostics.Monitoring.Tool.UnitTests/CollectDumpActionTests.cs

jander-msft · 2021-09-13T20:13:59Z

src/Tools/dotnet-monitor/CollectionRules/Actions/CollectDumpAction.cs

+        private readonly IDumpService _dumpService;
+        private readonly IServiceProvider _serviceProvider;
+
+        internal const string egressPath = "EgressPath";


Suggested change

internal const string egressPath = "EgressPath";

internal const string EgressPathOutputValueName = "EgressPath";

* Initial collection rule options, validation, and unit tests. (#688) * Generate action and trigger options in schema. (#706) Generate action and trigger options in schema. Add display name descriptions to these options. Add defaults for the optional properties. * Create `Execute` collection rule action (#710) * Basic implementation and registration of actions and triggers (#736) Initial interfaces for collection rule triggers. Implement empty action and trigger implementations. Implement startup trigger with special interface denoting it is a startup trigger. Rename action provider and trigger provider to descriptor. Register action and trigger implementations with descriptors. * Execute Action - Testing (#742) * Update trigger interfaces with callback, start, and stop notions. (#760) Update trigger interfaces with callback, start, and stop notions. Refactor options providers into operations implementations. * Refactor host building into helper method (#769) * Refactor host building into helper method. * Triggers - Action List Executor (#768) * Implement collection rule pipeline, event counter trigger, and tests. (#825) Implement collection rule pipeline and event counter trigger. Add tests for collection rule pipeline with real and test triggers and actions. * Create CollectDump Collection Rule Action (#824) * Fix CollectionRulePipelineTests failure due to callback registration timing. (#854) * Collection rule service, logging, and some functional tests (#852) * Implement collection rule service for starting rules. Add logging throughout collection rule system. Add some functional tests for collection rule system. * Refactor common parameters into CollectionRuleContext. * Add safe cancellation. * Add collection rule completion timeouts. * Refactor logging event IDs into separate shareable class. * Adjust temporary directory creation and log cleanup failures. * Remove CollectionRulePipeline.StartAsync; replace with callback. Add safe awaiting for start tasks and run tasks. Add more logging and adjust some log levels. * Replace TestCommon dependency with Xunit.Assert (#874) Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: kkeirstead <[email protected]> Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Wiktor Kopec <[email protected]> Co-authored-by: Patrick Fenelon <[email protected]>

kkeirstead added 17 commits August 23, 2021 13:33

Currently a complete mess; the execute action for completing a dump m…

1fd04e3

…ay be close to functional, but don't yet have tests to confirm that.

In the process of trying to get tests up and running; mostly just fig…

0d7c109

…uring out what needs to be pulled from existing tests and how to combine it to make a test that can collect a dump using ExecuteAsync.

Pulled in changes; in process of trying to get tests running for dump…

e43a2f4

…. Might end up waiting until Justin's change gets merged in for using IHost to get this resolved.

Dump Tests now successfully executes and outputs a file. Doesn't have…

ff5cc4d

… the .dmp suffix and not entirely sure whether it's working correctly, but exciting to see it function. Code is currently a mess and needs to be refined and trimmed.

Discovered that the file I was returning was the temporary one, and t…

ffc3f1b

…hat the permanent one is not being written. Need to investigate how that actually happens.

Realized that we aren't supporting the null scenario for egress; inte…

346af25

…rmediary check in

Merge branch 'feature/triggers' of https://github.com/dotnet/dotnet-m…

7649719

…onitor into kkeirstead/triggers/DumpAction

Merge branch 'feature/triggers' of https://github.com/dotnet/dotnet-m…

abff7cc

…onitor into kkeirstead/triggers/DumpAction

Not currently in a functioning state (for tests); need to ask Justin …

f24cc13

…about how to best integrate with existing infrastructure.

Removed a lot of commented out code that was no longer relevant and w…

8809244

…as hurting readability in tests. No functional changes.

File egress test is now passing and file is being egressed to the spe…

77c4619

…cified directory. Still need to do refactoring to better share code, and probably another test for azure blob egress.

Reorganization and some code clean-up.

7781356

Some more clean-up and commenting; decided not to add test for invali…

47f4568

…d egress provider due to built-in validation that is not used when running unit tests. Should enter PR soon.

Temp changes that should go in before PR; not passing tests though fo…

f67ff51

…r unknown reasons.

Small tweaks; test command is succeeding (may have been an issue in VS)

279c0e8

Merged with feature/triggers; currently having an issue where it won'…

d3804db

…t build because of complaints about netcore2.1

Removed an extraneous comment.

3e90a00

kkeirstead requested a review from a team as a code owner September 3, 2021 20:11

kkeirstead commented Sep 3, 2021

View reviewed changes

wiktork reviewed Sep 3, 2021

View reviewed changes

jander-msft requested changes Sep 3, 2021

View reviewed changes

kkeirstead and others added 3 commits September 7, 2021 14:57

Significant refactoring for Wiktor and Justin; overall functionality …

2e92313

…should be consistent with the initial commit.

Merge branch 'feature/triggers' into kkeirstead/triggers/DumpAction

00bd057

Resolving an error that resulted from GitHub's automatic merge.

29e1cad

kkeirstead and others added 5 commits September 8, 2021 10:10

This appears to have resolved the merge conflict issues (mostly surro…

bdc4847

…unding EndpointInfoSourceCallback and EndpointUtilities). Local tests are passing.

Merge branch 'feature/triggers' into kkeirstead/triggers/DumpAction

664aea2

Slight refactoring to get a shared method out of DiagController and i…

527fb58

…nto the Utilities class.

Merge branch 'kkeirstead/triggers/DumpAction' of https://github.com/k…

894c214

…keirstead/dotnet-monitor into kkeirstead/triggers/DumpAction

Still experimenting to get tests to pass. Issue appears to be with Fi…

da9d016

…leNotFound exceptions, so made some changes that bring this code to more closely resemble what's being done in the Egress tests. Tests still pass locally.

jander-msft requested changes Sep 10, 2021

View reviewed changes

wiktork reviewed Sep 10, 2021

View reviewed changes

Changes for Justin and Wiktor

6ff42bf

kkeirstead commented Sep 10, 2021

View reviewed changes

src/Tools/dotnet-monitor/CollectionRules/Actions/CollectDumpAction.cs Outdated Show resolved Hide resolved

jander-msft requested changes Sep 10, 2021

View reviewed changes

jander-msft reviewed Sep 10, 2021

View reviewed changes

src/Microsoft.Diagnostics.Monitoring.WebApi/DumpService.cs Show resolved Hide resolved

jander-msft reviewed Sep 10, 2021

View reviewed changes

src/Microsoft.Diagnostics.Monitoring.WebApi/DumpService.cs Outdated Show resolved Hide resolved

jander-msft reviewed Sep 10, 2021

View reviewed changes

src/Microsoft.Diagnostics.Monitoring.WebApi/DumpService.cs Outdated Show resolved Hide resolved

jander-msft reviewed Sep 10, 2021

View reviewed changes

src/Tests/Microsoft.Diagnostics.Monitoring.Tool.UnitTests/CollectDumpActionTests.cs Outdated Show resolved Hide resolved

kkeirstead added 2 commits September 10, 2021 15:23

More tweaks for Justin.

f707c78

Formatting tweaks and removing some unused using statements.

19ccda9

jander-msft approved these changes Sep 13, 2021

View reviewed changes

Rename for Justin.

6ffdb5b

kkeirstead merged commit 2ad6b1d into dotnet:feature/triggers Sep 14, 2021

kkeirstead deleted the kkeirstead/triggers/DumpAction branch November 7, 2022 16:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create CollectDump Collection Rule Action #824

Create CollectDump Collection Rule Action #824

kkeirstead commented Sep 3, 2021

kkeirstead Sep 3, 2021

wiktork Sep 3, 2021

wiktork Sep 3, 2021

wiktork Sep 3, 2021

wiktork Sep 3, 2021

wiktork Sep 3, 2021

wiktork Sep 3, 2021

kkeirstead commented Sep 7, 2021

wiktork Sep 10, 2021

wiktork Sep 10, 2021

jander-msft Sep 13, 2021


		IProcessInfo processInfo = await _diagnosticServices.GetProcessAsync(new ProcessKey(endpointInfo.ProcessId), token);

		string dumpFileName = DiagController.GenerateDumpFileName();


		string dumpFileName = DiagController.GenerateDumpFileName();

		string dumpFilePath = "";

	internal const string egressPath = "EgressPath";
	internal const string EgressPathOutputValueName = "EgressPath";

Create CollectDump Collection Rule Action #824

Create CollectDump Collection Rule Action #824

Conversation

kkeirstead commented Sep 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkeirstead commented Sep 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment