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

DiagnosticSourceEventSource supports base class properties #55613

Merged
merged 1 commit into from
Jul 14, 2021

Conversation

noahfalk
Copy link
Member

Fixes #41300

Previously DiagnosticSourceEventSource would only iterate and recognize properties that were declared on the most derived type.
Now it can capture properties that were inherited from a base class too.

@dotnet/dotnet-diag - can someone review?

Fixes dotnet#41300

Previously DiagnosticSourceEventSource would only iterate and recognize properties that were declared on the most derived type.
Now it can capture properties that were inherited from a base class too.
@ghost
Copy link

ghost commented Jul 14, 2021

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

Issue Details

Fixes #41300

Previously DiagnosticSourceEventSource would only iterate and recognize properties that were declared on the most derived type.
Now it can capture properties that were inherited from a base class too.

@dotnet/dotnet-diag - can someone review?

Author: noahfalk
Assignees: -
Labels:

area-System.Diagnostics.Tracing

Milestone: -

@davmason
Copy link
Member

EventSource also only looks at the derived type's fields and methods. I remember it being an intentional decision, but this was back in 2014ish and I can't remember why it was intentional. There was some scenario it broke to include the base class fields/methods.

Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

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

LGTM

@tarekgh
Copy link
Member

tarekgh commented Jul 14, 2021

There was some scenario it broke to include the base class fields/methods.

I can think if the base and inherited classes define non-virtual same property.

    public class A { public int P1 { get; set; } }
    public class B : A { public new int P1 { get; set; } }

@noahfalk
Copy link
Member Author

Thanks guys! EventSource requires every method to be annotated with [Event] or [NonEvent] so I could easily imagine including new methods would break the init logic for some pre-existing EventSource. I think that will be a fairly different scenario than this one. It is theoretically possible that someone wrote a DiagnosticSourceEventSource listener that will fail if more properties than expected are enumerated, but it seems unlikely:

  • The typical pattern is to use anonymous types for DiagnosticListener arguments in which case there are no additional base class properties to enumerate
  • ImplicitTransform is intended as a development time scenario when you don't know which properties you need, after which the tool author would switch to an explicit property list
  • Anyone who wrote code that breaks in this scenario would probably also break if a library author added a new property to their type. It would be a very version brittle technique with no benefit I am aware of so I am speculating folks wouldn't be doing it.

@stephentoub stephentoub merged commit afaf666 into dotnet:main Jul 14, 2021
thaystg added a commit to thaystg/runtime that referenced this pull request Jul 14, 2021
…debugger_custom_views

* 'main' of github.com:thaystg/runtime: (125 commits)
  [wasm] [debugger] Support method calls  (dotnet#55458)
  [debugger] Fix debugging after hot reloading (dotnet#55599)
  Inliner: Extend IL limit for profiled call-sites, allow inlining for switches. (dotnet#55478)
  DiagnosticSourceEventSource supports base class properties (dotnet#55613)
  [mono] Fix race during mono_image_storage_open (dotnet#55201)
  [mono] Add wrapper info for native func wrappers. (dotnet#55602)
  H/3 and Quic AppContext switch (dotnet#55332)
  Compression.ZipFile support for Unix Permissions (dotnet#55531)
  [mono] Fix skipping of static methods during IMT table construction. (dotnet#55610)
  Combine System.Private.Xml TrimmingTests projects (dotnet#55606)
  fix name conflict with Configuration class (dotnet#55597)
  Finish migrating RSAOpenSsl from RSA* to EVP_PKEY*
  Disable generic math (dotnet#55540)
  Obsolete CryptoConfig.EncodeOID (dotnet#55592)
  Address System.Net.Http.WinHttpHandler's nullable warnings targeting .NETCoreApp (dotnet#54995)
  Enable Http2_MultipleConnectionsEnabled_ConnectionLimitNotReached_ConcurrentRequestsSuccessfullyHandled (dotnet#55572)
  Fix Task.WhenAny failure mode when passed ICollection of zero tasks (dotnet#55580)
  Consume DistributedContextPropagator in DiagnosticsHandler (dotnet#55392)
  Add property ordering feature (dotnet#55586)
  Reduce subtest count in Reflection (dotnet#55537)
  ...
@ghost ghost locked as resolved and limited conversation to collaborators Aug 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DiagnosticSource does not get properties in base class
4 participants