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/7.0] Starting metrics before runtime start leads to a crash #81308

Merged
merged 3 commits into from
Feb 9, 2023

Conversation

dramos020
Copy link
Contributor

When you start a metrics session before the runtime is started, we will get a NullReferenceException in MetricsEventSource. This prevents metrics from being enabled before the process is started. The dotnet diagnostic tooling supports starting a session before the runtime is launched.

Customer Impact

Partner teams cannot ship diagnostic tooling on 6.0 without this change.

Testing

Manually verified that the fix works

Risk

Low.

…ine (dotnet#76965)

* Initial fix for null reference exception

* Took away lazy and put back private constructor

* Added parent property and made handler thread safe
@ghost
Copy link

ghost commented Jan 28, 2023

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

Issue Details

When you start a metrics session before the runtime is started, we will get a NullReferenceException in MetricsEventSource. This prevents metrics from being enabled before the process is started. The dotnet diagnostic tooling supports starting a session before the runtime is launched.

Customer Impact

Partner teams cannot ship diagnostic tooling on 6.0 without this change.

Testing

Manually verified that the fix works

Risk

Low.

Author: dramos020
Assignees: dramos020
Labels:

area-System.Diagnostics.Tracing

Milestone: -

@teo-tsirpanis teo-tsirpanis added this to the 7.0.x milestone Jan 28, 2023
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.

approved. we will take for consideration in 7.0.x

@jeffschwMSFT jeffschwMSFT added the Servicing-consider Issue for next servicing release review label Jan 28, 2023
@jeffschwMSFT jeffschwMSFT changed the title [release/7.0] Port #76965 to release/7.0 [release/7.0] Starting metrics before runtime start leads to a crash Jan 28, 2023
@rbhanda rbhanda modified the milestones: 7.0.x, 7.0.4 Jan 31, 2023
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jan 31, 2023
@carlossanlop
Copy link
Member

@dramos020 This assembly is an OOB package. Can you please add the necessary changes as described here? https://github.com/dotnet/runtime/blob/release/7.0/docs/project/library-servicing.md

@carlossanlop
Copy link
Member

@dramos020 I see this change and the 6.0 one were manually created. Was this merged to main first? If not, can you please make the change in that branch first?

@dramos020
Copy link
Contributor Author

@carlossanlop I made the changes to the servicing version. Also, these changes were merged to main earlier with this pr #76965

@carlossanlop
Copy link
Member

I fixed the merge conflict. It was caused by the reset of the OOB packages in the branding PR.

@carlossanlop
Copy link
Member

Since there was a failure found in the 6.0 backport that fixes this same issue, can you please confirm if the fix for that PR needs to go into this PR as well?

@carlossanlop
Copy link
Member

Since there was a failure found in the 6.0 backport that fixes this same issue, can you please confirm if the fix for that PR needs to go into this PR as well?

Based on the reply in the 6.0 PR, the issue should not happen in this 7.0 branch (it's fixed here already). #81307 (comment)

Approved by Tactics for 7.0.4.
Signed-off by area owner.
CI failures unrelated: #81619 #81544 and #74838
Required OOB changes look good.
Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit 0512a92 into dotnet:release/7.0 Feb 9, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants