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.

Sign up for GitHub

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

Jump to bottom

Supports collecting GCSettingsEvent for gc-collect profile #4615

Merged
merged 8 commits into from
May 10, 2024
Merged

Supports collecting GCSettingsEvent for gc-collect profile #4615

merged 8 commits into from
May 10, 2024

Conversation

cshung
Copy link
Member

@cshung cshung commented Apr 17, 2024

This change supports collecting GCSettingsEvent for gc-collect profile. The change will work for both dotnet-trace and dotnet-monitor.

The depends on the underlying mechanism built in dotnet/runtime#101189

@cshung cshung requested a review from a team as a code owner April 17, 2024 16:11
@cshung cshung mentioned this pull request Apr 17, 2024
Merged
@noahfalk
Copy link
Member

As part of this change please also update the protocol doc stored here: https://github.com/dotnet/diagnostics/blob/main/documentation/design-docs/ipc-protocol.md

@noahfalk
Copy link
Member

fyi @dotnet/dotnet-monitor

@cshung
Copy link
Member Author

cshung commented Apr 25, 2024

Test results on Windows.

Tool Arguments Rundown Behavior
dotnet monitor gc-collect (no rundown for 8, settings for 9)
  gc-collect + cpu (full rundown for 8, 9)
  gc-collect + metrics (no rundown for 8, settings for 9)
     
dotnet-trace gc-collect (no rundown for 8, settings for 9)
  gc-collect rundown true (full rundown for 8, 9)
  gc-collect rundown false (no rundown for 8, 9)

@cshung
Copy link
Member Author

cshung commented Apr 25, 2024

@noahfalk, can we take a look at this again? With the latest code, I should have addressed all the comments above.

noahfalk
noahfalk reviewed Apr 25, 2024
View reviewed changes
documentation/design-docs/ipc-protocol.md Outdated Show resolved Hide resolved
src/Microsoft.Diagnostics.Monitoring.EventPipe/Configuration/AggregateSourceConfiguration.cs Outdated Show resolved Hide resolved
src/Tools/dotnet-trace/CommandLine/Commands/CollectCommand.cs Outdated Show resolved Hide resolved
src/Tools/dotnet-trace/CommandLine/Commands/CollectCommand.cs Outdated Show resolved Hide resolved
src/Tools/dotnet-trace/CommandLine/Commands/CollectCommand.cs Outdated Show resolved Hide resolved
src/Microsoft.Diagnostics.Monitoring.EventPipe/EventPipeStreamProvider.cs Outdated Show resolved Hide resolved
src/Microsoft.Diagnostics.Monitoring.EventPipe/Configuration/MonitoringSourceConfiguration.cs Outdated Show resolved Hide resolved
src/Microsoft.Diagnostics.Monitoring.EventPipe/Configuration/MonitoringSourceConfiguration.cs Outdated Show resolved Hide resolved
src/Microsoft.Diagnostics.Monitoring.EventPipe/Configuration/MonitoringSourceConfiguration.cs Outdated Show resolved Hide resolved
src/Microsoft.Diagnostics.Monitoring.EventPipe/Configuration/AggregateSourceConfiguration.cs Outdated Show resolved Hide resolved
@noahfalk
Copy link
Member

@dotnet/dotnet-monitor you probably want to take a look at this

@cshung
Copy link
Member Author

cshung commented Apr 29, 2024

@noahfalk, here is the third iteration with all the comments addressed again.

noahfalk
noahfalk reviewed May 1, 2024
View reviewed changes
src/Microsoft.Diagnostics.Monitoring.EventPipe/Configuration/AggregateSourceConfiguration.cs Outdated Show resolved Hide resolved
src/Microsoft.Diagnostics.Monitoring.EventPipe/Configuration/MetricSourceConfiguration.cs Show resolved Hide resolved
src/Tools/dotnet-trace/CommandLine/Commands/CollectCommand.cs Outdated Show resolved Hide resolved
src/Tools/dotnet-trace/CommandLine/Commands/CollectCommand.cs Outdated Show resolved Hide resolved
src/Tools/dotnet-trace/CommandLine/Commands/CollectCommand.cs Outdated Show resolved Hide resolved
src/Microsoft.Diagnostics.Monitoring.EventPipe/EventPipeStreamProvider.cs Outdated Show resolved Hide resolved
src/Microsoft.Diagnostics.Monitoring.EventPipe/EventPipeStreamProvider.cs Outdated Show resolved Hide resolved
src/Microsoft.Diagnostics.Monitoring.EventPipe/EventPipeStreamProvider.cs Outdated Show resolved Hide resolved
src/Tools/dotnet-trace/CommandLine/Commands/CollectCommand.cs Outdated Show resolved Hide resolved
@cshung cshung force-pushed the public/gc-settings-event branch from 85d057c to 65fda18 Compare May 1, 2024 21:33
@cshung @noahfalk
Code Review Feedback (Round 3)
d680c3d 
Co-authored-by: Noah Falk <noahfalk@users.noreply.github.com>
@cshung cshung force-pushed the public/gc-settings-event branch from 65fda18 to d680c3d Compare May 2, 2024 18:36
@cshung cshung force-pushed the public/gc-settings-event branch from c03f6e6 to 76fa4a4 Compare May 9, 2024 21:58
noahfalk
noahfalk approved these changes May 10, 2024
View reviewed changes
Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

Modulo a couple small issues, this LGTM

src/Microsoft.Diagnostics.Monitoring.EventPipe/EventPipeStreamProvider.cs Outdated Show resolved Hide resolved
src/Microsoft.Diagnostics.NETCore.Client/DiagnosticsClient/EventPipeSession.cs Show resolved Hide resolved
src/Tools/dotnet-trace/CommandLine/Commands/CollectCommand.cs Outdated Show resolved Hide resolved
src/Microsoft.Diagnostics.NETCore.Client/DiagnosticsClient/EventPipeSessionConfiguration.cs Outdated Show resolved Hide resolved
@cshung cshung enabled auto-merge (squash) May 10, 2024 18:55
noahfalk
noahfalk approved these changes May 10, 2024
View reviewed changes
Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

👍

@cshung cshung merged commit 4e781d0 into dotnet:main May 10, 2024
18 checks passed
@cshung cshung deleted the public/gc-settings-event branch May 10, 2024 20:45
@hoyosjs hoyosjs mentioned this pull request Jun 8, 2024
Merged
hoyosjs added a commit that referenced this pull request Jun 8, 2024
@hoyosjs @dotnet-maestro
@richlander @mikem8361 @janvorli @verdie-g @tommcdon @mdh1418 @cshung @noahfalk @viewizard @mmitche
Merge main into release branch (#4723)
c1db600 
- Update Alpine versions (#4589)
- Followup 1ES template changes (#4596)
- Fix Test Results tab in github UI (#4607)
- Move the symstore repo to diagnostics (#4603)
- Fix recent Alpine repo build (#4612)
- Fix issue #4611 - Switch to cross build for Alpine (#4621)
- Add waithandle to --clrevents option (#4624)
- Fix SOS unit tests dump generation fixture (#4636)
- Fix CONTRACT return type (#4649)
- [SymbolStore] Add support to index .NET Framework Runtime debugging
modules (#4616)
- [test][SOS] Disable OtherCommands Dumpmt test on Alpine (#4656)
- Supports collecting GCSettingsEvent for gc-collect profile (#4615)
- Update ipc-protocol.md (#4658)
- Fixing a case where we should set the RundownKeyword (#4662)
- Fix versioning of symstore ported assets (#4670)
- Add ubuntu 22.04 testing. Update 6,7 and 8 runtime versions (#4676)
- Update locker action version to update node version (#4682)
- Use managed identity for blob upload of release assets (#4680)
- Remove the symweb symbol server support (#4694)
- [RISC-V] Add gcinfodumper implementation. (#4703)
- Fix `pathto` SOS command. (#4706)
- Add support for new exception trace storage format (#4635)
- Change SOS breaking change from warning to error; better dotnet-dump
target errors (#4713)
- [Symbol] Add manifest generator (#4693)
- Remove references to obsolete storage account variables (#4719)
- Update DIA to 17.10.0-beta1.24272.1 (#4683)
- Reenable SDL requirements (#4722)

---------

Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Rich Lander <rlander@microsoft.com>
Co-authored-by: Mike McLaughlin <mikem@microsoft.com>
Co-authored-by: Jan Vorlicek <janvorli@microsoft.com>
Co-authored-by: Grégoire <gregoire.verdier@gmail.com>
Co-authored-by: Tom McDonald <tommcdon@microsoft.com>
Co-authored-by: Mitchell Hwang <16830051+mdh1418@users.noreply.github.com>
Co-authored-by: Andrew Au <andrewau@microsoft.com>
Co-authored-by: Noah Falk <noahfalk@users.noreply.github.com>
Co-authored-by: Mikhail Kurinnoi <m.kurinnoi@samsung.com>
Co-authored-by: Matt Mitchell <mmitche@microsoft.com>
@github-actions github-actions bot locked and limited conversation to collaborators Jun 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@noahfalk noahfalk noahfalk approved these changes

@Maoni0 Maoni0 Maoni0 left review comments

@wiktork wiktork wiktork left review comments

@jander-msft jander-msft jander-msft left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@cshung @noahfalk @Maoni0 @wiktork @jander-msft