Skip to content

Proposed new System.Diagnostics APIs #32660

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

Closed
tarekgh wants to merge 6 commits into from
Closed

Proposed new System.Diagnostics APIs #32660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0033a42
Proposed new System.Diagnostics APIs
tarekgh Feb 21, 2020
502ea4c
Merge branch 'master' of https://github.com/dotnet/runtime into Syste…
tarekgh Feb 26, 2020
ed8e05a
Merge branch 'master' of https://github.com/dotnet/runtime into Syste…
tarekgh Feb 26, 2020
38e13df
Noah's Feedback
tarekgh Feb 27, 2020
4d401dc
small fix
tarekgh Feb 27, 2020
fb12851
Move AddListener() to ActivityListener.Start()
tarekgh Feb 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 56 additions & 1 deletion ...es/System.Diagnostics.DiagnosticSource/ref/System.Diagnostics.DiagnosticSourceActivity.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@
// Changes to this file must follow the https://aka.ms/api-review process.
// ------------------------------------------------------------------------------

using System.Collections.Generic;

namespace System.Diagnostics
{
public partial class Activity
public partial class Activity : IDisposable
Copy link
Copy Markdown
Member

@stephentoub stephentoub Feb 23, 2020

Choose a reason for hiding this comment

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

What does this mean for all activity use today where it's not being disposed? Are we introducing leaks?

Copy link
Copy Markdown
Member Author

@tarekgh tarekgh Feb 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We are not introducing a leak. This is added for simplifying the pattern of using Activity. I am going to add some code samples in the issue we'll use for the design review. here is some example quick example for what we currently have and what we are proposing:

// How Activity is used today:

var activityListener = new DiagnosticSource("Azure.Core.Http");

// Outer check to see if anyone is subscribed
if (activityListener.IsEnabled())
{
    Activity activity = null;
    // Check if anyone cares about activity
    if (activityListener.IsEnabled("Azure.Core.Http.Request"))
    {
        activity = new Activity("Azure.Core.Http.Request");
        activity.AddTag..
        activityListener.StartActivity(activity); // this does string concat and allocates every time
    }

    ...

    if (activity != null)
    {
        activityListener.StopActivity(activity);
    }
}

Here is what the new pattern will look like:

static ActivitySource source = new ActivitySource("Azure.Core.Http");

// ....

using (Activity activity = source.CraeteActivity())
{
        activity?.AddTag..
}

I am working to polish the sample code and add more samples.

{
public Activity(string operationName) { }
public System.Diagnostics.ActivityTraceFlags ActivityTraceFlags { get { throw null; } set { } }
Expand All @@ -34,6 +36,7 @@ public string? Id
get { throw null; }
}
public System.Diagnostics.ActivityIdFormat IdFormat { get { throw null; } }
public System.Diagnostics.ActivityKind Kind { get; set; }
public string OperationName { get { throw null; } }
public System.Diagnostics.Activity? Parent { get { throw null; } }
public string? ParentId { get { throw null; } }
Expand All @@ -43,9 +46,11 @@ public string? Id
public System.Diagnostics.ActivitySpanId SpanId { get { throw null; } }
public System.DateTime StartTimeUtc { get { throw null; } }
public System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, string?>> Tags { get { throw null; } }
public System.Collections.Generic.IEnumerable<ActivityLink> Links { get { throw null; } }
public System.Diagnostics.ActivityTraceId TraceId { get { throw null; } }
public string? TraceStateString { get { throw null; } set { } }
public System.Diagnostics.Activity AddBaggage(string key, string? value) { throw null; }
public System.Diagnostics.Activity AddLink(ActivityLink link) { throw null; }
public System.Diagnostics.Activity AddTag(string key, string? value) { throw null; }
public string? GetBaggageItem(string key) { throw null; }
public System.Diagnostics.Activity SetEndTime(System.DateTime endTimeUtc) { throw null; }
Expand All @@ -55,6 +60,9 @@ public string? Id
public System.Diagnostics.Activity SetStartTime(System.DateTime startTimeUtc) { throw null; }
public System.Diagnostics.Activity Start() { throw null; }
public void Stop() { }
public void Dispose() { }
public void SetCustomProperty(string propertyName, object? propertyValue) { }
Comment thread
tarekgh marked this conversation as resolved.
Outdated
public object? GetCustomProperty(string propertyName) { throw null; }
}
public enum ActivityIdFormat
{
Expand Down Expand Up @@ -121,4 +129,51 @@ public virtual void OnActivityImport(System.Diagnostics.Activity activity, objec
public System.Diagnostics.Activity StartActivity(System.Diagnostics.Activity activity, object? args) { throw null; }
public void StopActivity(System.Diagnostics.Activity activity, object? args) { }
}
public enum ActivityKind
{
Internal = 1,
Comment thread
noahfalk marked this conversation as resolved.
Outdated
Server = 2,
Client = 3,
Producer = 4,
Consumer = 5,
}
public readonly struct ActivityContext : IEquatable<ActivityContext>
{
public ActivityContext(System.Diagnostics.ActivityTraceId traceId, System.Diagnostics.ActivitySpanId spanId, System.Diagnostics.ActivityTraceFlags traceOptions, string? traceState = null) { throw null; }
public System.Diagnostics.ActivityTraceId TraceId { get; }
public System.Diagnostics.ActivitySpanId SpanId { get; }
public System.Diagnostics.ActivityTraceFlags TraceFlags { get; }
public string? TraceState { get; }
public static bool operator ==(System.Diagnostics.ActivityContext context1, System.Diagnostics.ActivityContext context2) { throw null; }
public static bool operator !=(System.Diagnostics.ActivityContext context1, System.Diagnostics.ActivityContext context2) { throw null; }
public bool Equals(System.Diagnostics.ActivityContext context) { throw null; }
public override bool Equals(object? obj) { throw null; }
public override int GetHashCode() { throw null; }
}
public readonly struct ActivityLink
{
public ActivityLink(System.Diagnostics.ActivityContext context) { throw null; }
public ActivityLink(System.Diagnostics.ActivityContext context, System.Collections.Generic.IDictionary<string, object>? attributes) { throw null; }
public System.Diagnostics.ActivityContext Context { get; }
public System.Collections.Generic.IDictionary<string, object>? Attributes { get; }
}
public sealed class ActivitySource : IDisposable
Copy link
Copy Markdown

@lmolkova lmolkova Feb 21, 2020

Choose a reason for hiding this comment

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

we discussed that for libs (Azure SDK) it's not a problem to create ActivitySource per operation, but it seems it could a problem/typical mistake for app developers.

E.g. imagine in the controller you want to track custom Activities. Developers would have to create static ActivitySource and I guess it will be common forget/not read docs and do this:

using (var s = new ActivitySource("foo"))
using (var a = s.CreateActivity())
{
  // do stuff
}

Can we think about it from app dev convenience?

Copy link
Copy Markdown
Member Author

@tarekgh tarekgh Feb 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

does the concern here ActivitySource is Disposable? if so, we can think in another way to dispose it. but wouldn't anyone writing a code as you showed will not work for them anyway and will it be obvious there is a problem?
One idea we can provide a method like Untrack() (or whatever name we can agree on) and we'll remove the IDisposable interface.

static ActivitySource s = new ActivitySource("foo");
...
using (var a = s.CreateActivity())
{
  // do stuff
}

...
// later if the app want to get rid of it will do 
s.Untrack();

what you think?

Copy link
Copy Markdown
Member

@noahfalk noahfalk Feb 24, 2020

Choose a reason for hiding this comment

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

If an app wrote @lmolkova's example I don't see any reason that it wouldn't function properly? The only downside of that pattern is the app author is paying the cost to create the ActivitySource, publish it, and have telemetry agents decide if they want to subscribe to it on each Activity generated.

Can we think about it from app dev convenience?

If the concern is being able to write simple one liners or not reading the docs then I'd guess the most likely risk is not calling Dispose() on the ActivitySource. For example:

    using(var a = new ActivitySource("foo").CreateActivity())
    {
    }

Looking at it a bit I believe we could eliminate the need for Dispose() by making a global list of weak GCHandles rather than a global list of strong ActivitySource references. We pay one additional pointer of memory per ActivitySource for the GCHandle and need a little more complicated implementation. ActivitySource is already around 100 bytes each (~4 pointers + 3 pointers for string + ~20*2 characters) so another pointer doesn't appear to be a significant relative overhead.

Copy link
Copy Markdown

@lmolkova lmolkova Feb 24, 2020

Choose a reason for hiding this comment

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

The only downside of that pattern is the app author is paying the cost to create the ActivitySource, publish it, and have telemetry agents decide if they want to subscribe to it on each Activity generated.

This is exactly my concern: the easiest and most convenient way to use this API requires 2 allocations and listener to start/stop subscription in e.g. per-request basis (imagine concurrency issues and locks in listener).

Assuming source is less granular that activity, it's possible to have 1) default source and you create one per app or web host 2) source per e.g. controller that you can register as singleton in your container.

With current implementation it's not even possible to register source in DI container (if you want more than one) and users will be forced to have static fields to keep each source.

Copy link
Copy Markdown
Member Author

@tarekgh tarekgh Feb 24, 2020

Choose a reason for hiding this comment

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

A couple of clarification questions to get the whole picture:

  • Does the ask to make it easier to create a source, publish and listen to the custom activities without the need to hold the source in a static field (or store it somewhere)?
  • if the answer is yes, does it require at any point later you can retrieve the exactly created source instance to do something else with it? or don't care about the source after establishing the event listeners?

Copy link
Copy Markdown
Member

@noahfalk noahfalk Feb 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Assuming source is less granular that activity, it's possible to have 1) default source and you create one per app or web host 2) source per e.g. controller that you can register as singleton in your container.

I think similar to @tarekgh I am trying to understand your goal : ) Is the goal of the default source to allow for simpler usage like this?

    using(Activity a = Activity.Start("activityName"))
    {
    }

The source per controller I am less sure what the goal is there?

With current implementation it's not even possible to register source in DI container

Is your concern that if we don't then it won't appeal to developers writing ASP.Net apps? Something else?

Copy link
Copy Markdown

@SergeyKanzhelev SergeyKanzhelev Mar 3, 2020

Choose a reason for hiding this comment

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

with the design with so many allocations - is the assumption that the span name for ASP.NET incoming requests will be a constant value like "HttpIn"? In OpenTelemetry there is a notion of a name and for http it is typically the "route name". So we either need a notion of sub-name or Activity.Name is actually a "component name". And we are building a spec for "component"

Copy link
Copy Markdown
Member Author

@tarekgh tarekgh Mar 3, 2020

Choose a reason for hiding this comment

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

We are going to get rid of ActivitySource all together. I am going to share the new final proposal by tomorrow or so.

Copy link
Copy Markdown

@SergeyKanzhelev SergeyKanzhelev Mar 3, 2020

Choose a reason for hiding this comment

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

Can we call ActivitySource Tracer in the new proposal?

Copy link
Copy Markdown
Member Author

@tarekgh tarekgh Mar 3, 2020

Choose a reason for hiding this comment

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

We are going to get rid of ActivitySource all together.

{
private ActivitySource() { throw null; }
public ActivitySource(string name) { throw null; }
public string Name { get; }
public Activity? StartActivity() { throw null; }
public Activity? StartActivity(System.Diagnostics.ActivityContext context, System.Collections.Generic.IEnumerable<ActivityLink>? links = null, System.DateTimeOffset startTime = default) { throw null; }
public void Dispose() { }
}
public abstract class ActivityListener : IDisposable
{
public void Start() { throw null; }
public virtual bool EnableListening(string activitySourceName) { throw null; }
public virtual bool ShouldCreateActivity(string activitySourceName, ActivityContext context, IEnumerable<ActivityLink>? links) { throw null; }
public virtual void OnActivityStarted(Activity a) { throw null; }
public virtual void OnActivityStopped(Activity a) { throw null; }
public void Dispose() { throw null; }
protected virtual void Dispose(bool disposing) { throw null; }
}
}
60 changes: 33 additions & 27 deletions src/libraries/System.Diagnostics.DiagnosticSource/src/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
<?xml version="1.0" encoding="utf-8"?>
<root>
<!--
Microsoft ResX Schema
<!--
Microsoft ResX Schema

Version 2.0
The primary goals of this format is to allow a simple XML format
that is mostly human readable. The generation and parsing of the
various data types are done through the TypeConverter classes

The primary goals of this format is to allow a simple XML format
that is mostly human readable. The generation and parsing of the
various data types are done through the TypeConverter classes
associated with the data types.

Example:

... ado.net/XML headers & schema ...
<resheader name="resmimetype">text/microsoft-resx</resheader>
<resheader name="version">2.0</resheader>
Expand All @@ -26,36 +26,36 @@
<value>[base64 mime encoded string representing a byte array form of the .NET Framework object]</value>
<comment>This is a comment</comment>
</data>
There are any number of "resheader" rows that contain simple

There are any number of "resheader" rows that contain simple
name/value pairs.
Each data row contains a name, and value. The row also contains a
type or mimetype. Type corresponds to a .NET class that support
text/value conversion through the TypeConverter architecture.
Classes that don't support this are serialized and stored with the

Each data row contains a name, and value. The row also contains a
type or mimetype. Type corresponds to a .NET class that support
text/value conversion through the TypeConverter architecture.
Classes that don't support this are serialized and stored with the
mimetype set.
The mimetype is used for serialized objects, and tells the
ResXResourceReader how to depersist the object. This is currently not

The mimetype is used for serialized objects, and tells the
ResXResourceReader how to depersist the object. This is currently not
extensible. For a given mimetype the value must be set accordingly:
Note - application/x-microsoft.net.object.binary.base64 is the format
that the ResXResourceWriter will generate, however the reader can

Note - application/x-microsoft.net.object.binary.base64 is the format
that the ResXResourceWriter will generate, however the reader can
read any of the formats listed below.

mimetype: application/x-microsoft.net.object.binary.base64
value : The object must be serialized with
value : The object must be serialized with
: System.Runtime.Serialization.Formatters.Binary.BinaryFormatter
: and then encoded with base64 encoding.

mimetype: application/x-microsoft.net.object.soap.base64
value : The object must be serialized with
value : The object must be serialized with
: System.Runtime.Serialization.Formatters.Soap.SoapFormatter
: and then encoded with base64 encoding.

mimetype: application/x-microsoft.net.object.bytearray.base64
value : The object must be serialized into a byte array
value : The object must be serialized into a byte array
: using a System.ComponentModel.TypeConverter
: and then encoded with base64 encoding.
-->
Expand Down Expand Up @@ -144,9 +144,15 @@
<data name="SetFormatOnStartedActivity" xml:space="preserve">
<value>"Can not change format for an activity that was already started"</value>
</data>
<data name="SetLinkInvalid" xml:space="preserve">
<value>"Can not add link to activity after it has been started"</value>
</data>
<data name="SetParentIdOnActivityWithParent" xml:space="preserve">
<value>"Can not set ParentId on activity which has parent"</value>
</data>
<data name="SpanIdOrTraceIdInvalid" xml:space="preserve">
<value>"Invalid SpanId or TraceId"</value>
</data>
<data name="StartTimeNotUtc" xml:space="preserve">
<value>"StartTime is not UTC"</value>
</data>
Expand Down
5 changes: 5 additions & 0 deletions ...raries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@
<Link>Common\System\HexConverter.cs</Link>
</Compile>
<Compile Include="System\Diagnostics\Activity.cs" />
<Compile Include="System\Diagnostics\ActivityContext.cs" />
<Compile Include="System\Diagnostics\ActivityKind.cs" />
<Compile Include="System\Diagnostics\ActivityLink.cs" />
<Compile Include="System\Diagnostics\ActivitySource.cs" />
<Compile Include="System\Diagnostics\ActivityListener.cs" />
<Compile Include="System\Diagnostics\DiagnosticSourceActivity.cs" />
<Reference Include="System.Memory" />
<Reference Include="System.Runtime.CompilerServices.Unsafe" />
Expand Down
Loading