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

[release/6.0] Starting metrics before runtime start leads to a crash #81307

Merged
merged 3 commits into from
Feb 10, 2023
Merged

[release/6.0] Starting metrics before runtime start leads to a crash #81307

merged 3 commits into from
Feb 10, 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.

@dramos020
Fixes issue when monitoring a process launched via the same command l…
441298c 
…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 ghost assigned dramos020 Jan 28, 2023
@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: -

@dramos020 dramos020 changed the title [release 6.0] Port #76965 to release 6.0 [release/6.0] Port #76965 to release/6.0 Jan 28, 2023
@teo-tsirpanis teo-tsirpanis added this to the 6.0.x milestone Jan 28, 2023
@jeffschwMSFT jeffschwMSFT changed the title [release/6.0] Port #76965 to release/6.0 [release/6.0] Starting metrics before runtime start leads to a crash Jan 28, 2023
jeffschwMSFT
jeffschwMSFT approved these changes Jan 28, 2023
View reviewed changes
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 6.0.x

@jeffschwMSFT jeffschwMSFT added the Servicing-consider Issue for next servicing release review label Jan 28, 2023
@rbhanda rbhanda modified the milestones: 6.0.x, 6.0.15 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 is an OOB package. Can you please follow these instructions to ensure we build it properly? https://github.com/dotnet/runtime/blob/release/6.0/docs/project/library-servicing.md

@carlossanlop carlossanlop mentioned this pull request Feb 7, 2023
Merged
@carlossanlop
Copy link
Member

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

carlossanlop
carlossanlop approved these changes Feb 8, 2023
View reviewed changes
Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

OOB changes look good. Thanks for adding them, @dramos020.

@dramos020
Copy link
Contributor Author

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

@carlossanlop
Copy link
Member

these changes were merged to main earlier with this pr #76965

Thanks for sharing that info, @dramos020. We ask backport submitters to mention the main PR number in the backport PR description.

Not sure if you know this, but in case you don't, it might be helpful - If you type this text as a comment in your main PR:

/backport to release/6.0

The github-actions bot will create the 6.0 backport PR for you with all the info filled out in the description. This feature works with all servicing and preview branches.

Example of a main PR: #81070 (comment)
Example of a generated backport: #81139

@carlossanlop
Copy link
Member

@dramos020 can you please investigate the CI failures in ~System.Diagnostics.Metrics.Tests.MetricsTests`? They might be related to this PR.

@dramos020
Copy link
Contributor Author

Yup, looking into it right now

@carlossanlop carlossanlop mentioned this pull request Feb 8, 2023
Closed
@carlossanlop carlossanlop added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 8, 2023
@hoyosjs
Copy link
Member

hoyosjs commented Feb 9, 2023

The issue is that the assembly load fails because the ref is still 6.0.0.0 but the src assembly is 6.0.0.1. @carlossanlop @ViktorHofer, do we rev ref assemblies, or is the fix here to use SkipUseReferenceAssembly="true" in the test project reference instead?

@ViktorHofer
Copy link
Member

ViktorHofer commented Feb 9, 2023 •
edited
Loading

The issue is that the assembly load fails because the ref is still 6.0.0.0 but the src assembly is 6.0.0.1. @carlossanlop @ViktorHofer, do we rev ref assemblies, or is the fix here to use SkipUseReferenceAssembly="true" in the test project reference instead?

Yes, and that got fixed in main and release/7.0 with #78703 and #78730. There's not an easy way to backport the change. We worked around these issues in the past by checking in binding redirects into the .NET Framework test project. Please try that. Here's an example: #78250

@carlossanlop
Copy link
Member

@dramos020 can you please follow up on Viktor's suggestion?

@dramos020
Copy link
Contributor Author

dramos020 commented Feb 9, 2023 •
edited
Loading

I tried Viktor's suggestion, but the tests are still failing.

@dramos020
Copy link
Contributor Author

Juan's suggestion of adding SkipUseReferenceAssembly="true" to the test project reference worked. The System.Diagnostics.Metrics.Tests.MetricsTests should now be passing

@ViktorHofer
Copy link
Member

Binding redirects definitely should have worked, maybe you conditioned it on the wrong TargetFramework? Setting SkipUseReferenceAssembly works here as well. Both options are hacks / workarounds so I don't feel strongly about either.

@carlossanlop carlossanlop removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 10, 2023
@carlossanlop
Copy link
Member

CI is green. Ready to merge.

@carlossanlop carlossanlop merged commit e013e98 into dotnet:release/6.0 Feb 10, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@carlossanlop carlossanlop carlossanlop approved these changes

@jeffschwMSFT jeffschwMSFT jeffschwMSFT approved these changes

Assignees

@dramos020 dramos020

Labels
area-System.Diagnostics.Tracing Servicing-approved Approved for servicing release
Projects
None yet
Milestone
6.0.15
Development

Successfully merging this pull request may close these issues.
7 participants
@dramos020 @carlossanlop @hoyosjs @ViktorHofer @jeffschwMSFT @teo-tsirpanis @rbhanda