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

Support TraceSource to be initialized from the app.config file #73087

Merged
merged 10 commits into from
Aug 10, 2022
Merged

Support TraceSource to be initialized from the app.config file #73087

merged 10 commits into from
Aug 10, 2022

Conversation

steveharter
Copy link
Member

@steveharter steveharter commented Jul 29, 2022 •
edited
Loading

Fixes #23937 (original issue)
Fixes #72967 (API)

Much of the code was ported from .NET Framework and refactored to support the three new events to allow the config system to update TraceSource and friends from the config file(s). Original port per @ericstj in the first commit.

To enable this functionality, TraceConfiguration.Register() must be called. This is a new API that is not present in .NET Framework.

There is potentially more tests around error handling that could be added:

  • Bad XML validation.
  • Validation and happy path validation of the Attributes + GetSupportedAttributes() pattern in Switch, TraceSource and TraceListener.
  • Invalid constructor arguments in XML passed via initializeData.

@steveharter steveharter self-assigned this Jul 29, 2022
@ghost
Copy link

ghost commented Jul 29, 2022

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

Issue Details

WIP

Author: steveharter
Assignees: steveharter
Labels:

area-System.Diagnostics
Milestone: -

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@steveharter steveharter force-pushed the addDiagConfigSteve branch 2 times, most recently from b186226 to ba39523 Compare August 3, 2022 21:07
@steveharter steveharter added this to the 7.0.0 milestone Aug 4, 2022
@steveharter steveharter marked this pull request as ready for review August 4, 2022 18:56
@ericstj
Copy link
Member

ericstj commented Aug 4, 2022

cc @RussKie @JeremyKuhne since you're involved from Winforms side. @noahfalk was also involved from diagnostics side.

@RussKie
Copy link
Member

RussKie commented Aug 4, 2022

Thank you.
From the description is not obvious whether consumers will be required to change the code to get it to work, or it will work seamlessly as before.

@steveharter
Copy link
Member Author

Thank you.
From the description is not obvious whether consumers will be required to change the code to get it to work, or it will work seamlessly as before.

Good point - I'll add that to the description.

Customers will be required to call TraceConfiguration.Register()

noahfalk
noahfalk approved these changes Aug 9, 2022
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.

This LGTM. I think our WinForms users are really going to appreciate it!

...ource/tests/System.Diagnostics.TraceSource.Config.Tests/TraceSourceWithConfigurationTests.cs Outdated Show resolved Hide resolved
...s/System.Configuration.ConfigurationManager/ref/System.Configuration.ConfigurationManager.cs Outdated Show resolved Hide resolved
@steveharter steveharter force-pushed the addDiagConfigSteve branch 2 times, most recently from 9d815cf to 71f73e5 Compare August 9, 2022 23:57
@steveharter steveharter mentioned this pull request Aug 10, 2022
Open
@steveharter
Copy link
Member Author

Note to reviewers: there was supposedly support for a filemappingsize config entry in the System.Diagnostics section that was read by performance counters. However, that code was not functional so it was removed as explained in the newly created issue #73706 and in this code

@steveharter
Copy link
Member Author

CI is green with exception of runtime-staging.

@steveharter steveharter merged commit 9bd2cb5 into dotnet:main Aug 10, 2022
@steveharter steveharter deleted the addDiagConfigSteve branch August 10, 2022 21:32
@akoeplinger akoeplinger mentioned this pull request Aug 11, 2022
Closed
Tanya-Solyanik
Tanya-Solyanik reviewed Aug 12, 2022
View reviewed changes
src/libraries/System.Diagnostics.TraceSource/src/System/Diagnostics/Switch.cs
internal void OnInitializing()
{
Initializing?.Invoke(null, new InitializingSwitchEventArgs(this));
TraceUtils.VerifyAttributes(Attributes, GetSupportedAttributes(), this);
Copy link
Member

Choose a reason for hiding this comment

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

This call causes Winforms test to fail - dotnet/winforms#7578

@Tanya-Solyanik Tanya-Solyanik mentioned this pull request Aug 12, 2022
Merged
@KlausLoeffelmann KlausLoeffelmann mentioned this pull request Aug 14, 2022
Merged
@danmoseley
Copy link
Member

Do we need to do anything to help folks that are trying to port discover how to make this work? Is it sufficient to hope they find the original issue and navigate here?

Should we put an example in the RC1 release post?

@RussKie
Copy link
Member

RussKie commented Aug 16, 2022

I believe @KlausLoeffelmann was able to devise a fix, so I think we're good now.

@danmoseley
Copy link
Member

@RussKie what I'm asking is how a customer porting to .NET Core whose code fails at runtime with something similar to

Unhandled Exception: System.Configuration.ConfigurationErrorsException: Configuration system failed to initialize ---> System.Configuration.ConfigurationErrorsException: Unrecognized configuration section system.diagnostics.

... will learn that they need to use .NET 7+ and add a call to TraceConfiguration.Register()..?

@KlausLoeffelmann is not a typical customer.

This was referenced Aug 17, 2022
Closed
Open
@steveharter steveharter mentioned this pull request Aug 30, 2022
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Sep 15, 2022
@steveharter steveharter added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Nov 2, 2022
@ghost ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Nov 2, 2022
@steveharter
Copy link
Member Author

Breaking change doc: dotnet/docs#32147

@steveharter steveharter removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Nov 2, 2022
@RussKie
Copy link
Member

RussKie commented Nov 2, 2022

@steveharter just to confirm - the fix is only in .NET 7 and it's not available in the earlier versions, right?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@RussKie RussKie RussKie left review comments

@ViktorHofer ViktorHofer ViktorHofer left review comments

@Tanya-Solyanik Tanya-Solyanik Tanya-Solyanik left review comments

@noahfalk noahfalk noahfalk approved these changes

@ericstj ericstj Awaiting requested review from ericstj

@JeremyKuhne JeremyKuhne Awaiting requested review from JeremyKuhne

@carlossanlop carlossanlop Awaiting requested review from carlossanlop

Assignees

@steveharter steveharter

Labels
area-System.Diagnostics breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. new-api-needs-documentation
Projects
None yet
Milestone
7.0.0
7 participants
@steveharter @ericstj @RussKie @danmoseley @noahfalk @ViktorHofer @Tanya-Solyanik