Skip to content

Conversation

@Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Jul 26, 2020

@Youssef1313 Youssef1313 requested review from a team and sdmaclea as code owners July 26, 2020 18:22
@dotnet-bot dotnet-bot added this to the July 2020 milestone Jul 26, 2020
Copy link
Contributor

@tdykstra tdykstra left a comment

Choose a reason for hiding this comment

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

This is a good start -- I had a few doc structure and wording suggestions. I would put this in the TOC following dotnet-dump

dotnet-gcdump collect [-h|--help] [-p|--process-id <pid>] [-o|--output <gcdump-file-path>] [-v|--verbose] [-t|--timeout <timeout>] [-n|--name <name>]
```

### Options
Copy link
Contributor

Choose a reason for hiding this comment

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

Following the pattern of https://raw.githubusercontent.com/dotnet/docs/master/docs/core/diagnostics/dotnet-counters.md we should have an Examples h3 section following each Options h3 section.


- **`-t|--report-type <HeapStat>`**

The type of report to generate. Available options: heapstat (default).
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is the only option, what's the difference between specifying it an omitting it?

Copy link
Member Author

Choose a reason for hiding this comment

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

@tdykstra, I had this question myself. It looks like there shouldn't be a difference. Perhaps the tool team might be planning to add other options later?

Copy link
Contributor

Choose a reason for hiding this comment

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

/cc @josalem

Copy link
Contributor

Choose a reason for hiding this comment

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

This is only report type available right now and therefore the default, but we created this flag for future use if/when we add more report types.

@Youssef1313
Copy link
Member Author

Youssef1313 commented Jul 28, 2020

The options here are in the same order produced by the CLI help, do you want me to order them alphabetically as done with dotnet-trace? @tdykstra


The PR still lacks examples. I'll try adding some.

@tdykstra
Copy link
Contributor

The options here are in the same order produced by the CLI help, do you want me to order them alphabetically as done with dotnet-trace?

There's no reason the docs have to follow the CLI help order; any way we can make the docs more usable seems worth doing to me. In this case the small number of options makes the benefit of limited value, but my preference is to be consistent.

Copy link
Contributor

@tdykstra tdykstra left a comment

Choose a reason for hiding this comment

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

@Youssef1313
Copy link
Member Author

deliberate

I'll add these. Thanks for noticing this.

Co-authored-by: Petr Kulikov <petr.kulikov@gmail.com>
@sdmaclea sdmaclea requested a review from josalem July 31, 2020 16:42

- **`-t|--report-type <HeapStat>`**

The type of report to generate. Available options: heapstat (default).
Copy link
Contributor

Choose a reason for hiding this comment

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

This is only report type available right now and therefore the default, but we created this flag for future use if/when we add more report types.

@adegeo
Copy link
Contributor

adegeo commented Aug 17, 2020

@tdykstra Is this good to go or do you have other concerns?

Copy link
Contributor

@tdykstra tdykstra left a comment

Choose a reason for hiding this comment

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

A couple suggestions


The type of report to generate. Available options: heapstat (default).

## Viewing the GC dump captured from dotnet-gcdump
Copy link
Contributor

Choose a reason for hiding this comment

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

This might fit as an H3 in the Description section

@tdykstra
Copy link
Contributor

There's still a markdownlint error - we can merge this when that's resolved.

The PR still lacks examples. I'll try adding some.

You can save that for a later PR if you prefer.

@Youssef1313
Copy link
Member Author

There's still a markdownlint error - we can merge this when that's resolved.

The PR still lacks examples. I'll try adding some.

You can save that for a later PR if you prefer.

I completely forgot about that. If it's okay to merge this, I'll open an issue to track adding examples. Otherwise, I'll find time to add them tomorrow or so.

@tdykstra
Copy link
Contributor

Yes OK to merge and add later. Thanks for creating this doc!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Missing docs for dotnet-gcdump

8 participants