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

[release/9.0] Root the System.Runtime EventSource #108348

Merged
merged 2 commits into from
Sep 30, 2024

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Sep 27, 2024

Backport of #108266 to release/9.0

/cc @noahfalk

Customer Impact

Starting in RC1 runtime metrics available via dotnet-counters, dotnet-monitor, Visual Studio and potentially other tools were missing if the tool wasn't started quickly after app startup. Once the metrics were missing they are unrecoverable for the remainder of the process lifetime.

The underlying cause is that the RuntimeEventSource object which provides the counters was being garbage collected because it isn't properly rooted. The timing on how quickly the counters go away depend on how quickly the GC runs.

Regression

  • Yes
  • No

Regressed starting in RC1, caused by #106014

Testing

I manually tested the change using the repro provided in the issue report.

Risk

Low - the change restores behavior that was present before the regression, its a tiny code change that is easy to reason about, and its been tested.

The System.Runtime EventSource (RuntimeEventSource), was unintentionally being garbage collected because it wasn't rooted. This caused runtime EventCounters to no longer be available after GC occurred.

This was a regression from a recent change (#106014). That change accidentally converted the static field that was intended to the root the object into a property getter that returned a new instance each time it was called. This fix converts the property back into a statically initialized field.

This will fix #107919 once it is backported.
@tarekgh tarekgh added the Servicing-consider Issue for next servicing release review label Sep 27, 2024
@tarekgh
Copy link
Member

tarekgh commented Sep 27, 2024

CC @artl93 @ericstj

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

lgtm. we will take for consideration in 9 GA

Copy link
Contributor

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

@jeffschwMSFT jeffschwMSFT added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 30, 2024
@tarekgh
Copy link
Member

tarekgh commented Sep 30, 2024

/ba-g no real failure in the build analysis. The CI just skipped one leg.

@tarekgh
Copy link
Member

tarekgh commented Sep 30, 2024

Could someone have permission help merge this PR? Thanks!

@tommcdon
Copy link
Member

Could someone have permission help merge this PR? Thanks!

I believe @jeffschwMSFT has merge permissions

@jeffschwMSFT jeffschwMSFT merged commit dc412cb into release/9.0 Sep 30, 2024
147 of 153 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants