Skip to content

Add ProcessorCount Event to RuntimeEventSource #65581

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
Merged
mikelle-rogers merged 4 commits into from
Jun 29, 2022
Merged

Add ProcessorCount Event to RuntimeEventSource #65581

mikelle-rogers merged 4 commits into from
Jun 29, 2022

Conversation

@mikelle-rogers
Copy link
Member

@mikelle-rogers mikelle-rogers commented Feb 18, 2022

fixes #56143

@mikelle-rogers mikelle-rogers added this to the 7.0.0 milestone Feb 18, 2022
@mikelle-rogers mikelle-rogers self-assigned this Feb 18, 2022
@ghost
Copy link

ghost commented Feb 18, 2022

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

fixes #56143

Author: mikelle-rogers
Assignees: mikelle-rogers
Labels:

area-System.Diagnostics
Milestone: 7.0.0

@mikelle-rogers mikelle-rogers marked this pull request as ready for review March 2, 2022 19:44
davmason
davmason approved these changes Apr 4, 2022
View reviewed changes
Copy link
Contributor

@davmason davmason left a comment

Choose a reason for hiding this comment

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

LGTM

@davmason davmason closed this Apr 4, 2022
@davmason davmason reopened this Apr 4, 2022
@mikelle-rogers mikelle-rogers force-pushed the dev/mirogers/EventSource-ProcessCount branch from cd0a8c6 to cf0db13 Compare April 28, 2022 20:04
josalem
josalem approved these changes May 10, 2022
View reviewed changes
Copy link
Contributor

@josalem josalem left a comment

Choose a reason for hiding this comment

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

Could we add a test for this event?

e.g., Assert.Equal(Environment.ProcessorCount, countFromEvent);

@mikelle-rogers mikelle-rogers requested a review from davmason May 18, 2022 23:18
@tommcdon
Copy link
Member

tommcdon commented Jun 29, 2022

Adding @brianrob in case TraceEvent changes are needed.

@brianrob
Copy link
Member

brianrob commented Jun 29, 2022

@tommcdon, thanks for including me here. This doesn't require a change to TraceEvent because we don't have a parser for this provider - it is emitted via the standard EventSource mechanism.

This does bring up a question for me. Historically, if I understand correctly, this source was used exclusively for event counters. I see that now the source also logs app context switches, and now the processor count. How should I think about the split between this provider and the Microsoft-Windows-DotNetRuntime provider?

@mikelle-rogers mikelle-rogers merged commit 84b2320 into dotnet:main Jun 29, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jul 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoub stephentoub stephentoub approved these changes

@noahfalk noahfalk Awaiting requested review from noahfalk

+2 more reviewers

@davmason davmason davmason approved these changes

@josalem josalem josalem approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mikelle-rogers mikelle-rogers

Labels

area-System.Diagnostics

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

[Feature Request]: Diagnostics for processor count

7 participants

@mikelle-rogers @tommcdon @brianrob @stephentoub @noahfalk @davmason @josalem