dotnet / diagnostics

This repository contains the source code for various .NET Core runtime diagnostic tools and documents.
MIT License
1.18k stars 354 forks source link

Add support for .netconfig in dotnet-counters #1536

Open kzu opened 4 years ago

kzu commented 4 years ago

The dotnetconfig format allows hierarchical tool settings to be persisted and read in a standards way. By adding this support to the counters tool, all options can now be persisted to the .netconfig in the current directory to streamline subsequent runs.

This is very convenient since when doing performance investigations, it's not unusual to perform the same collection over and over, where the only change across runs is the process being inspected. Depending on the type of investigation typically performed by the user, they might even be the same settings always for any .NET process, so the settings could be shared for the entire user by just saving the .netconfig to the user profile directory instead (or some ancestor folder from where it's typically run).

.netconfig is already used by a bunch of other tools and it seems like a good fit for this tool, and maybe others that involve many arguments that may be relatively constant across runs (such as for dotnet-serve).

The way it would work would be as follows (copied from initial implementation):

Configuration with .netconfig

In order to avoid repeating lengthy command line arguments every time the tool is used, you can leverage the built-in support for .netconfig. This allows all tool arguments to be saved in .netconfig files (which support hierarchical configuration too) so they can be reused across tool runs.

All settings must be placed under the counters section, for example:

[counters]
    refresh-interval = 5
    format = json

The counter_list argument, supported in both monitor and collect commands, can be used as unqualified provider names, specified directly in the counters section with include:

[counters]
    include = System.Runtime
    include = Microsoft.AspNetCore.Hosting

Or they can be qualified by including the relevant counter names in their own subsections:

[counters]
    include = Microsoft.AspNetCore.Hosting

[counters "System.Runtime"]
    cpu-usage
    working-set
    assembly-count
    exception-count

The above configuration will include all counters (i.e. unqualified) from the Microsoft.AspNetCore.Hosting provider, and only the specified counters for the System.Runtime provider.

The configured values are used as fallback default values when the corresponding argument isn't provided via the command line.

kzu commented 4 years ago

I have the implementation ready as soon as #1534 is implemented (since I based it on that).

kzu commented 4 years ago

If this is deemed useful, I think it can be made uniformly supported across all the other diagnostic tools, like:

[dump]
    name = foo                            # should this be process-name uniformly too?
    output = ...
    diag                                  # shorthand notation in .netconfig/gitconfig for true variable value
    command = ...

[gcdump]
    name = foo                            # should this be process-name uniformly too?
    output = ...
    verbose
    timeout = 15

[monitor]
    url = http://foo.com/...
    url = https://bar.com/....            # many URLs can be added this way with multi-valued vars
    metricUrl = http://localhost:52325    # looks like this is also a plural/multi-valued var
    reversed-server-address = ...

[sos]
    architecture = ...

[trace]
    # collect command
    buffersize = 250KB                    # .netconfig provides built-in support for size multiplier suffixes in numbers
    output = ...
    provider = ...                        # multi-valued var here too
    provider = ...
    profile-name = ...
    duration = 00:00:05:00                # No need to quote anything for the most part :)
    clrevent = ...                        # multi-valued var
    clrevent = ...
    clrevent = ...
    clreventlevel = ...

    # convert command
    format = NetTrace                     # NetTrace, Speedscope, Chromium
    output = ...                          
josalem commented 4 years ago

Besides being a structured format with a parsing API, does this have any intrinsic advantage over just using a response file? Currently you can accomplish 90% of this by just writing your arguments into a file and calling the tool like so:

# myconfig.rsp

# Listen to MyAwesomeProvider and MyOtherProvider
--providers MyAwesomeProvider:5:fffffff:MyKey=MyValue;MyOtherKey=MyOtherValue,MyOtherProvider

# Use a 1 GB buffer
--buffersize 1024

# trace for 10 minutes
--duration 00:00:10:00

dotnet-trace collect -p 1234 @myconfig.rsp

It isn't automatically picked up but allows for simple config sharing and persistence.

To be clear, I'm not opposed to making these changes, but we need to do some info gathering on preferred formats and mechanisms before adopting something as important as a configuration mechanism.

@shirhatti & @noahfalk do you have any thoughts on this config format and associated API?

kzu commented 4 years ago

The intrinsic advantages are many:

  1. Hierarchical configuration you can reuse from all parent directories, user profile and system locations
  2. Automatic support for per-user, local-only configs that are (mostly) automatically ignored when committing changes (via .netconfig.user files). So team-wide settings can be checked-in, while user-only can go to a separate file
  3. No need to know which response file to pass in, a single one can feed values to multiple tools (response files need to be specific for each tool, since otherwise args would not be recorgnized)
  4. Avoid proliferation of multitude of files when multiple tools are used over time in a given repo
  5. Uniformity regardless of what CLI parsing implementation is being used by the tool author: .netconfig has already been integrated in dotnet-serve, reportgenerator, sleet, dotnet-vs and others. Not all CLI parsing libraries support response files, and the dev might not even be using one. Adding .netconfig support is very simple and more flexible and powerful than adding support for response files, I think :)
  6. .netconfig leverages the learnings and long history of multi-tool configuration from gitconfig (it's the same format), so it's not really a new thing, from that point of view
  7. In cases where multiple tools share common argument names that mean the same (i.e. process-name in this case, maybe other props?), it's trivial to avoid repeating that by having a common section and share the value (i.e. [diagnostics] process-name = ..., say). That is, again, only doable with response files if you split into multiple files with things that are shared vs non-shared, and then remember to pass them in every time. Not quite as convenient.
  8. Ability to edit the .netconfig independently of the tools that consume them, using dotnet-config, which provides the same (and even 100% compatible) CLI as git config so they can be edited by either too.
  9. You even get nice syntax highlighting in GH markdown if you mark the fenced code blocks with the gitconfig language ;) (see example). Might be worth contributing the dotnetconfig alias to that grammar to linguist :)

😍

noahfalk commented 4 years ago

Hi @kzu thanks for all your work on this : ) I hate to be the guy who rains on the parade, but I'd recommend we not take this change right now and we can reconsider in the future. There are a few reasons but the top ones:

kzu commented 4 years ago

Hey @noahfalk, no worries 👍

  1. IMHO, having significant feedback should not be the sole driver for enhacements that bring in entirely new features. Even if the feedback is not specific to one particular tool, bringing consistency across dotnet global tools (a nascent but I think growing and potentially fairly large) ecosystem early on can only be a good thing. Granted, not all tools will need configuration-driven CLIs, but some definitely will (i.e. I can't imagine that repetitive typing for those counter provider filters being an enjoyable thing), and supporting a single way when configuration makes sense is a good thing. In particular, these diagnostic tools are rather new, so I would expect the feedback to not be significant (in volume) anyway...

  2. Not really. One thing I'm exploring is seeing if it can be added at a lower level, to the System.CommandLine library. In that case, it would not add another UI. Maybe for maximum control of the mapping to config you might still want to directly use the API (such as in the counter list in this case, which already is a bit of a custom format/mechanism for the multi-values it can receive... So, I guess if the mapping was "automatic" (say with an optional command/option/arg attribute-based annotation for tuning?), would you say this makes it an easier "yes" choice?

  3. I'd like to approach the dotnet foundation for dotnetconfig. Would that make this an easier choice too?

Thanks!

noahfalk commented 4 years ago

So, I guess if the mapping was "automatic" would you say this makes it an easier "yes" choice?

Yeah, if the configuration is predictable/automatic given the CLI syntax that resolves that part of my concern. I was only commenting on what the current state of affairs appeared to be. If things change or I misunderstood I can happily update my position.

I'd like to approach the dotnet foundation for dotnetconfig. Would that make this an easier choice too?

Depending on the nature of the relationship it might be able to help in some areas, but there is always going to be some unavoidable scrutiny here. When taking on a new dependency I assume tool developers are going to be thinking in varying degrees about size, performance, licensing, future support/maintenance, security, compliance, and standards.

Thanks @kzu!

kzu commented 4 years ago

I forgot to mention another amazing benefit: since it's compatible with gitconfig, you can use fenced code blocks marked with that language and get colorization that works both in GH markdown rendering (see https://github.com/dotnetconfig/dotnet-config#format), which uses linguist, as well as docfx (see https://dotnetconfig.org/#format) which uses highligh.js ;)