Add Support for Dump File Name Templates #6675

wsugarman · 2024-05-18T02:32:49Z

Summary

Enable users to configure the name of artifacts for the following actions:

CollectDump
CollectExceptions
CollectGCDump
CollectLiveMetrics
CollectLogs
CollectStacks
CollectTrace

Furthermore, the following placeholders have been added to bring parity with the placeholders used by the environment variable DOTNET_DbgMiniDumpName/COMPlus_DbgMiniDumpName as detailed here.

Specifier	Value
`$(Process.Hostname)`	Host name return by `gethostname()`
`$(Action.UnixTime)`	Time of the action, expressed as seconds since the Epoch, 1970-01-01 00:00:00 +0000 (UTC)

Resolve #3917

Release Notes Entry

Add support for configuring the artifact name
Add new placeholders $(Process.Hostname) and $(Action.UnixTime)

wsugarman · 2024-05-18T02:34:22Z

I have not yet added tests. I thought I'd first put out a first draft to gauge interest and assess the overall design. I'm no expert on this tool nor this project, so please provide any and all feedback.

src/Tools/dotnet-monitor/Dumps/DumpFilePathTemplate.cs

wiktork

Some design considerations:

We may want to have some uniformity around token substitutions. I.e. doing $(Process.ProcessId) in one context but %p in another can be confusing.
Having a one-off configuration for dumps works, but we'll need to consider if this should be more generic and apply to other artifacts
Should the setting be possible to provide at an api level? Or strictly configuration.

wsugarman · 2024-05-21T18:49:03Z

Some design considerations:

We may want to have some uniformity around token substitutions. I.e. doing $(Process.ProcessId) in one context but %p in another can be confusing.

Having a one-off configuration for dumps works, but we'll need to consider if this should be more generic and apply to other artifacts

Should the setting be possible to provide at an api level? Or strictly configuration.

Thank you for the thoughts!

The first point makes sense to me. I am having trouble tracking down in the code or docs where this syntax is already used for things like $(Process.ProcessId). Could you provide a link?
I agree, especially because the artifact name concept is used across artifact types.
It could make sense in the web APIs. The only tricky part is that the artifact appears to be typically returned to the user via the HTTP response unless otherwise indicated, like specifying egress for the /dump route. Then the settings for the egress are all made via a configuration instead of via the request. In other words, by default, the name may be irrelevant and only becomes relevant once a custom egress is specified. Given that existing egress settings are specified via configuration, I would reckon that artifact name should be the same. However, artifact name is a concept that is separate from each egress type, so maybe it makes more sense to be configurable per request?

wiktork · 2024-05-24T00:16:08Z

The first point makes sense to me. I am having trouble tracking down in the code or docs where this syntax is already used for things like $(Process.ProcessId). Could you provide a link?

https://github.com/dotnet/dotnet-monitor/blob/main/src/Tools/dotnet-monitor/ConfigurationTokenParser.cs

This class would likely require some refactoring to support scalar string values instead of properties

It could make sense in the web APIs.

I think we could do this piece wise. Start with just configuration and consider an api level override later.

This reverts commit 8020845.

This reverts commit 1408cf6.

This reverts commit 0081dfd.

wsugarman · 2024-06-03T00:31:28Z

The first point makes sense to me. I am having trouble tracking down in the code or docs where this syntax is already used for things like $(Process.ProcessId). Could you provide a link?

https://github.com/dotnet/dotnet-monitor/blob/main/src/Tools/dotnet-monitor/ConfigurationTokenParser.cs

This class would likely require some refactoring to support scalar string values instead of properties

It could make sense in the web APIs.

I think we could do this piece wise. Start with just configuration and consider an api level override later.

I've changed the pattern. Let me know what you think. I'm uncertain about two things:

The name of the placeholders $(Action.UnixTime and $(Process.Hostname)
The hostname isn't really that of the process; it's for the monitor. If we want the hostname, if there a remote scenario for dotnet-monitor, then we may need to retrieve it remotely.

…otnet-monitor into dotnet-dump-template

wiktork · 2024-06-07T21:36:03Z

src/Tools/dotnet-monitor/CollectionRules/ActionOptionsDependencyAnalyzer.cs

@@ -159,7 +160,9 @@ public object SubstituteOptionValues(IDictionary<string, CollectionRuleActionRes
                RuntimeId = _ruleContext.EndpointInfo?.RuntimeInstanceCookie ?? Guid.Empty,
                ProcessId = _ruleContext.EndpointInfo?.ProcessId ?? 0,
                CommandLine = commandLine,
-                ProcessName = _ruleContext.ProcessInfo?.ProcessName ?? string.Empty
+                ProcessName = _ruleContext.ProcessInfo?.ProcessName ?? string.Empty,
+                Hostname = Dns.GetHostName(),


nit: Would it be better to use Environment.MachineName?

As you pointed out, this would be dotnet-monitor's machine name, which is not what we are looking for. We can get the target process environment variables, that should be enough for the host name in aks and Windows scenarios. Not sure about Linux though.

jander-msft · 2024-06-07T21:47:12Z

src/Tools/dotnet-monitor/ConfigurationTokenParser.cs


        public static readonly string RuntimeIdReference = CreateTokenReference(ProcessInfoReference, RuntimeId);
        public static readonly string ProcessIdReference = CreateTokenReference(ProcessInfoReference, ProcessId);
        public static readonly string ProcessNameReference = CreateTokenReference(ProcessInfoReference, ProcessName);
        public static readonly string CommandLineReference = CreateTokenReference(ProcessInfoReference, CommandLine);
+        public static readonly string HostnameReference = CreateTokenReference(ProcessInfoReference, Hostname);


As it is, this variable (Process.Hostname) is semantically referring to the hostname of the target process. But the callers are collecting the hostname information of the host that .NET Monitor is running on. While these might be the same in many cases, I think this should be either:

properly classified with a new prefix (e.g. Monitor.Hostname) if using the hostname on which .NET Monitor is running

or, get the environment block from the target process and resolve the host name from that. See https://github.com/dotnet/dotnet-monitor/blob/main/src/Microsoft.Diagnostics.Monitoring.WebApi/Controllers/DiagController.cs#L178 as an example of how to get the environment block of the target process.

wiktork · 2024-06-07T21:51:04Z

src/Tools/dotnet-monitor/ConfigurationTokenParser.cs


        public static readonly string RuntimeIdReference = CreateTokenReference(ProcessInfoReference, RuntimeId);
        public static readonly string ProcessIdReference = CreateTokenReference(ProcessInfoReference, ProcessId);
        public static readonly string ProcessNameReference = CreateTokenReference(ProcessInfoReference, ProcessName);
        public static readonly string CommandLineReference = CreateTokenReference(ProcessInfoReference, CommandLine);
+        public static readonly string HostnameReference = CreateTokenReference(ProcessInfoReference, Hostname);
+        public static readonly string UnixTimeReference = CreateTokenReference(ActionReference, UnixTime);


I think we should use a different category for Timestamp than Action. These typically refer to other actions in a chain.

wiktork · 2024-06-07T21:52:21Z

src/Tools/dotnet-monitor/CollectionRules/Options/Actions/CollectDumpOptions.cs

+        [Display(
+            ResourceType = typeof(OptionsDisplayStrings),
+            Description = nameof(OptionsDisplayStrings.DisplayAttributeDescription_CollectArtifactOptions_ArtifactName))]
+        public string ArtifactName { get; set; }


I'm ok with this level of configuration, but I think long term we'll want a higher tier of configuration on a per artifact type rather than per egress type. We can add that later though.

wsugarman added 3 commits May 17, 2024 19:11

Initial design

0081dfd

Add proper unix seconds support

1408cf6

Fix default

8020845

wsugarman requested a review from a team as a code owner May 18, 2024 02:32

Merge branch 'main' into dotnet-dump-template

82e7193

dotnet-policy-service bot added the needs-review label May 18, 2024

wsugarman commented May 18, 2024

View reviewed changes

src/Tools/dotnet-monitor/Dumps/DumpFilePathTemplate.cs Outdated Show resolved Hide resolved

wiktork reviewed May 20, 2024

View reviewed changes

dotnet-policy-service bot removed the needs-review label May 20, 2024

wsugarman added 6 commits June 1, 2024 19:02

Merge remote-tracking branch 'origin/main' into dotnet-dump-template

2ae837f

Add host and time

38c3071

Revert "Fix default"

29fd72a

This reverts commit 8020845.

Revert "Add proper unix seconds support"

4b46fde

This reverts commit 1408cf6.

Revert "Initial design"

40024fb

This reverts commit 0081dfd.

Change pattern

680381e

dotnet-policy-service bot added the needs-review label Jun 3, 2024

Merge branch 'main' into dotnet-dump-template

ab86bd5

wsugarman added 2 commits June 2, 2024 17:55

Use default artifact name for API

ccad5c3

Merge branch 'dotnet-dump-template' of https://github.com/wsugarman/d…

adf6cc8

…otnet-monitor into dotnet-dump-template

wiktork reviewed Jun 7, 2024

View reviewed changes

dotnet-policy-service bot removed the needs-review label Jun 7, 2024

jander-msft reviewed Jun 7, 2024

View reviewed changes

wiktork reviewed Jun 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Support for Dump File Name Templates #6675

Add Support for Dump File Name Templates #6675

wsugarman commented May 18, 2024 •

edited

Loading

wsugarman commented May 18, 2024

wiktork left a comment

wsugarman commented May 21, 2024

wiktork commented May 24, 2024

wsugarman commented Jun 3, 2024

wiktork Jun 7, 2024

wiktork Jun 7, 2024

jander-msft Jun 7, 2024

wiktork Jun 7, 2024

wiktork Jun 7, 2024

Add Support for Dump File Name Templates #6675

Are you sure you want to change the base?

Add Support for Dump File Name Templates #6675

Conversation

wsugarman commented May 18, 2024 • edited Loading

Summary

Release Notes Entry

wsugarman commented May 18, 2024

wiktork left a comment

Choose a reason for hiding this comment

wsugarman commented May 21, 2024

wiktork commented May 24, 2024

wsugarman commented Jun 3, 2024

wiktork Jun 7, 2024

Choose a reason for hiding this comment

wiktork Jun 7, 2024

Choose a reason for hiding this comment

jander-msft Jun 7, 2024

Choose a reason for hiding this comment

wiktork Jun 7, 2024

Choose a reason for hiding this comment

wiktork Jun 7, 2024

Choose a reason for hiding this comment

wsugarman commented May 18, 2024 •

edited

Loading