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.

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

Enable Custom Artifact Names with New Tokens #6675

Merged
merged 26 commits into from
Aug 5, 2024

Conversation

wsugarman
Copy link
Contributor

@wsugarman wsugarman commented May 18, 2024

Summary

Enable users to configure the artifact name for the following actions:

  • CollectDump
  • CollectExceptions
  • CollectGCDump
  • CollectLiveMetrics
  • CollectLogs
  • CollectStacks
  • CollectTrace

Furthermore, the following tokens have been added to bring parity with createdump and the environment variable DOTNET_DbgMiniDumpName/COMPlus_DbgMiniDumpName used in .NET 5 and beyond, as detailed here.

Specifier Value
$(Monitor.HostName) Host name returned by gethostname()
$(Monitor.UnixTime) Time of the trigger, expressed as seconds since the Epoch, 1970-01-01 00:00:00 +0000 (UTC)

To support these new tokens, HostInfo has been added to the CollectionRuleContext that combines both the existing TimeProvider property (that used to be present directly on CollectionRuleContext) and a new HostName property.

Resolve #3917

Original Release Description (modified below for conciseness):

  • Add support for overriding the artifact name in the following actions:
    • CollectDump
    • CollectExceptions
    • CollectGCDump
    • CollectLiveMetrics
    • CollectLogs
    • CollectStacks
    • CollectTrace
  • Add new placeholders $(Monitor.HostName) and $(Monitor.UnixTime)
Release Notes Entry

Adds support for overriding the artifact name for Collect* actions and adds new placeholders for HostName and UnixTime.

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

@wiktork wiktork left a comment

Choose a reason for hiding this comment

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

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

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
Copy link
Member

wiktork commented May 24, 2024

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.

@wsugarman
Copy link
Contributor Author

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.

@dotnet-policy-service dotnet-policy-service bot added the no-recent-activity Automatically added by bot after four weeks of inactivity label Jul 6, 2024
@dotnet-policy-service dotnet-policy-service bot added needs-review and removed no-recent-activity Automatically added by bot after four weeks of inactivity labels Jul 7, 2024
@wsugarman wsugarman changed the title Add Support for Dump File Name Templates Enable Custom Artifact Names with New Tokens Jul 7, 2024
jander-msft
jander-msft previously approved these changes Jul 31, 2024
@wsugarman
Copy link
Contributor Author

Thank you for the approval @jander-msft! I have a few more comments from @wiktork, so I wait for you to approve as well to ensure I haven't miss any of your concerns. (I also don't have permission to merge PRs, so I'll need someone to ultimately merge it when it's ready 😅 )

@jander-msft jander-msft merged commit dc66aa0 into dotnet:main Aug 5, 2024
26 checks passed
@kkeirstead kkeirstead added the update-release-notes Pull requests that should be mentioned in the release notes label Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
update-release-notes Pull requests that should be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Customize artifact names
4 participants